{"action":"created","issue":{"url":"https://api.github.com/repos/octokit/octokit.net/issues/1269","repository_url":"https://api.github.com/repos/octokit/octokit.net","labels_url":"https://api.github.com/repos/octokit/octokit.net/issues/1269/labels{/name}","comments_url":"https://api.github.com/repos/octokit/octokit.net/issues/1269/comments","events_url":"https://api.github.com/repos/octokit/octokit.net/issues/1269/events","html_url":"https://github.com/octokit/octokit.net/pull/1269","id":148036577,"number":1269,"title":"[WIP] Begin implementation of Enterprise ManagementConsole API","user":{"login":"ryangribble","id":5425163,"avatar_url":"https://avatars.githubusercontent.com/u/5425163?v=3","gravatar_id":"","url":"https://api.github.com/users/ryangribble","html_url":"https://github.com/ryangribble","followers_url":"https://api.github.com/users/ryangribble/followers","following_url":"https://api.github.com/users/ryangribble/following{/other_user}","gists_url":"https://api.github.com/users/ryangribble/gists{/gist_id}","starred_url":"https://api.github.com/users/ryangribble/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ryangribble/subscriptions","organizations_url":"https://api.github.com/users/ryangribble/orgs","repos_url":"https://api.github.com/users/ryangribble/repos","events_url":"https://api.github.com/users/ryangribble/events{/privacy}","received_events_url":"https://api.github.com/users/ryangribble/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"shiftkey","id":359239,"avatar_url":"https://avatars.githubusercontent.com/u/359239?v=3","gravatar_id":"","url":"https://api.github.com/users/shiftkey","html_url":"https://github.com/shiftkey","followers_url":"https://api.github.com/users/shiftkey/followers","following_url":"https://api.github.com/users/shiftkey/following{/other_user}","gists_url":"https://api.github.com/users/shiftkey/gists{/gist_id}","starred_url":"https://api.github.com/users/shiftkey/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shiftkey/subscriptions","organizations_url":"https://api.github.com/users/shiftkey/orgs","repos_url":"https://api.github.com/users/shiftkey/repos","events_url":"https://api.github.com/users/shiftkey/events{/privacy}","received_events_url":"https://api.github.com/users/shiftkey/received_events","type":"User","site_admin":true},"assignees":[{"login":"shiftkey","id":359239,"avatar_url":"https://avatars.githubusercontent.com/u/359239?v=3","gravatar_id":"","url":"https://api.github.com/users/shiftkey","html_url":"https://github.com/shiftkey","followers_url":"https://api.github.com/users/shiftkey/followers","following_url":"https://api.github.com/users/shiftkey/following{/other_user}","gists_url":"https://api.github.com/users/shiftkey/gists{/gist_id}","starred_url":"https://api.github.com/users/shiftkey/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shiftkey/subscriptions","organizations_url":"https://api.github.com/users/shiftkey/orgs","repos_url":"https://api.github.com/users/shiftkey/repos","events_url":"https://api.github.com/users/shiftkey/events{/privacy}","received_events_url":"https://api.github.com/users/shiftkey/received_events","type":"User","site_admin":true}],"milestone":null,"comments":2,"created_at":"2016-04-13T11:55:58Z","updated_at":"2016-08-15T09:02:18Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/octokit/octokit.net/pulls/1269","html_url":"https://github.com/octokit/octokit.net/pull/1269","diff_url":"https://github.com/octokit/octokit.net/pull/1269.diff","patch_url":"https://github.com/octokit/octokit.net/pull/1269.patch"},"body":"This is the beginnings of adding support for #1024 the GitHub Enterprise [Management Console API](https://developer.github.com/v3/enterprise/management_console/)\r\n\r\nTo begin with Ive only added the Get/Set Maintenance Mode functions and the actual plumbing around how we can actually integrate with the management console API.\r\n\r\nIm aware there are merge conflicts as I havent rebased to latest master yet.  We are far from done with this PR so Ill do that in good time.  For now I just want to get the [WIP] up to find out from @Haacked and @shiftkey how far off track i've taken things :trollface: : \r\n\r\n### Notes\r\nThis API is significantly different to all the \"regular\" github APIs.  There are special requirements around connectivity and authentication and it also doesnt have fully fleshed out calls that take multiple parameters, but in more than one place simply have a single parameter that takes a json string specifying multiple parameters.  To top it off it had some really strange handling around date formats and some other things.\r\n\r\nIm definitely after guidance on some of the design decisions ive made...\r\n\r\n### Summary\r\nRead below for large detail on the inner workings and design decisions I made, but in a nut-shell someone using octokit.net against a GitHub Enterprise server an now simply call the following methods on the same `GitHubClient` client they have already created for regular API access.\r\n\r\n``` csharp\r\n// Some examples...\r\n\r\nvar consolePassword = \"MyPassword\";\r\n\r\n// Get maintenace status\r\nvar maintenance = await \r\n    _github.Enterprise.ManagementConsole.GetMaintenanceMode(consolePassword);\r\n\r\n// Set maintenance mode ON now\r\nvar maintenance = await\r\n    _github.Enterprise.ManagementConsole.EditMaintenanceMode(\r\n        new UpdateMaintenanceRequest(true, MaintenanceDate.Now()),\r\n        EnterpriseHelper.ManagementConsolePassword);\r\n\r\n// Set maintenance mode OFF\r\nvar maintenance = await\r\n    _github.Enterprise.ManagementConsole.EditMaintenanceMode(\r\n        new UpdateMaintenanceRequest(),\r\n        consolePassword);\r\n\r\n// Schedule maintenance for next wednesday 5pm\r\nvar maintenance = await\r\n    _github.Enterprise.ManagementConsole.EditMaintenanceMode(\r\n        new UpdateMaintenanceRequest(true, MaintenanceDate.FromChronicValue(\"Next wednesday at 5pm\")),\r\n        consolePassword);\r\n\r\n// Schedule maintenance mode in 10 minutes, with a DateTimeOffset\r\nvar start = DateTimeOffset.Now.AddMinutes(10);\r\nvar maintenance = await \r\n    _github.Enterprise.ManagementConsole.EditMaintenanceMode(\r\n        new UpdateMaintenanceRequest(true, MaintenanceDate.FromDateTimeOffset(start)),\r\n        consolePassword);\r\n```\r\n\r\n#### Network access\r\n- TCP Port\r\nSince octokit handles following redirects, we don't have any issues around handling the special ports of `8080` and `8443` so we are good on this front and can simply target this API at the regular server address the user is passing into the `GitHubClient` as octokit will follow redirects as needed\r\n\r\n- API Uri's\r\nNot as lucky on the Uri front though... The management console api calls must be targetted at `http://my.enterprise.server/setup/api` ie they can't have the `/api/v3/` in the URL as all other calls do.\r\nUnfortunatley in the guts of octokit.net's `GitHubClient` and `Connection` classes, this `/api/v3/` is added to any custom URL specified by the user and saved in the `baseUri` of the `Connection` object.\r\nOriginally my implementation made it so that you had to declare a new/special `GitHubClient` using a new constructor essentially stating that you wanted to use it for management console access, and this essentially passed a `bool` through which prevented the `/api/v3/` being put in the uri.  But of course this then meant that all the other API clients on this `GitHubClient` wouldnt actually work... and similarly it meant that the `client.Enterprise.ManagementConsole` client wouldn't work on a \"regular\" `GitHubClient` you were using to call the normal APIs.\r\nMy latest iteration was actually exploiting behaviour of the dotnet `Uri` appending behaviour, where if an endpoint relative Uri starts with a leading forward slash, it actually causes any Uri on the `baseUri` to be stripped off.  We've encountered the odd bug over time where inadvertently including these leading slashes on Urls in the `ApiUri` helper class has caused endpoints to not work on github enterprise, but now it seemed like this behaviour might actually help us here.  Rather than just magially include the leading slashes on the management console Uris, i implemented a function `CorrectEndpointForManagementConsole(endpoint)` that is called in each management console client method, to add the leading slash if not already there.  This does seem a bit hacky but really it is a fairly reliable way to allow a user to specify one `GitHubClient` object and credentials that work against the regular APIs on their enterprise instance, then when they use the ManagementConsole API on the same client, we manipulate the Uri via the endpoint (ie not manipulating the baseUri) and provide the management console password each time.  Plus there is zero behavioral change for anyone using octokit against github.com (ie not an enterprise user).  As an octokit contributor and enterprise user I felt like this has achieved good usability and is an acceptable code base compromise.  At least it's a starting point to see how @shiftkey and others feel :)\r\n\r\n#### Authenticating\r\nThe management console has a special password to access it.  For API use, this password is specified as the api key, however it appears it cant be specified in the \"normal\" way the user/password or OAUTH token can be (ie using our `Credentials` object which inserts it in a header in base64/UTF8 form).  I was only able to get things successfully working, by having `?api_key=MyPassword` on the end of the Uri (or potentially it would also work being embedded in the server name `https://api_key:MyPassword@hostname` but that causes similar problems as above if we want to try and have the one `GitHubClient` object able to work with all APIs\r\nSo with the way i've implemented it on the end of the Url, it actually doesnt work too badly as it means the existing credentials provided on creating the `GitHubClient` can stay, and are used for regular API access, and every management console method takes a `managementConsolePassword` parameter.  I actually like this as it's more inline with the seriousness of some of these management console activities - eg along the lines of \"provide your password everytime you do these management related things\"\r\nThe irksome thing is that the password is essentiall in clear text on the end of the Url but im not sure if that's really any different to it being in the HTTP headers for regular calls anyway...\r\n\r\n#### What the heck is up with parameters containing json strings?!!\r\nThe Set Maintenance Mode call takes a single `maintenance` parameter which is defined as follows:\r\n> *maintenance* `string` _Required_ A JSON string with the attributes enabled and when.\r\n\r\nI tried and tried and I just could NOT get things working in the normal way where we have a request object, which gets serialized to json and put in the body of the HTTP Post.  I had many incarnations providing a class with a public `Maintenance` parameter that then contained the c# object class with the `Enabled` and `When` parameters on it.  When that didnt work I tried simply having a public `string` Getter parameter that deserialized the class to json (didnt work), so then moved to constructing the json string in a more basic way - both normal format and fully escaped, I tried doing the parameter dictionary type approach like used in search requests, I tried the default content type and `application/json` content types but in all cases github would throw a HTTP 400 Bad Request.  \r\nEventually I found that if I just specified the string in the url-form-encoded format of `maintenace=\"{\"enabled\":true, \"when\":\"now\"}\"` it worked, so that's where I'm at at the moment.  I plan to tidy up the json string to not be created manually and instead call a serializer but for now I have something working so didnt want to wait in pushing it up.\r\nIf anyone is able to show me how to get this working in a more \"normal\" way for octokit where the request object is serialized using the builtin `SimpleJsonSerializer` in the normal pipeline, I would love to see what I missed bercause I feel like I tried so many attempts and still couldnt crack it without going the form encoded route.\r\n\r\nThis is definitely important to sort out a strategy for, because the [Settings](https://developer.github.com/v3/enterprise/management_console/#retrieve-settings) endpoint on the management console is a real doozy containing literally 50+ items in a hierarchical json structure that all have to get packed into a single json string on a `settings` property in the API call.  I am NOT looking forward to that endpoint!\r\n\r\n#### What the heck is up with the time handling?!!\r\nThe response to a scheduled maintenance mode event includes this field \r\n`\"scheduled_time\": \"Tuesday, January 22 at 15 => 34 -0800\"`\r\nIn any other github API call ive encountered, the dates are in a lovely and easily parseable ISO 8601 format \r\nIn this case we not only have the the full day and month names (this will pose issues if github can run in other cutures/languages), the silly formatting like comma and `=>` the non standard timezone offset information (in dotnet land this should be `-08:00` rather than `-0800` but most importantly THE YEAR is missing!  This plays havoc with attempting to actually parse this into a date.  Now if we hardcode a culture like `en-US` we can get around the full day/month names, and we can \"almost\" use `ParseExact()` to match most of the formatting issues, except the lack of year means we cant use `DateTimeOffset.ParseExact()` but if we use `DateTime.ParseExact()` the time zone offset is not handled well.  \r\nGiven that the day of week and month are mentioned, we \"could\" potentially take an educated guess at the year, since eg if it says Tuesday the 12th April, we can figure out what YEAR in the future the 12th April will fall on a Tuesday and presume it to be this... then we could shove that year into the string.  This is clearly getting ridiculous though (espcially because this field is coming back from the API and we want it to be deserialised properly in the Json deserializer) so I took the easy way out and simply made it a `string`!!!\r\nIn runtime testing this behaviour it's even more odd because through the API I can scheduled maintenance for a date in 1/1/2017 and the API response `Sunday, January 1 at 20:00` indicates it really is that 2017 date - the fact it says Sunday is the confirmation.  Yet the management console UI will show it as 1/1/2016 but in a scheduled status.  If it really was 20-16 we should either now BE in maintenance mode (not scheduled) or it should reject the update since it's in the past.  So im thinking even the management console UI itself is using this API call with the missing date and just assuming it's the current year.\r\nNow does it make sense to schedule maintenance for so far in the future?  Not really!  But given the API lets you do it, it's not really our place to limit this in any way.  My hope is that this API call gets modified to include a properly parseable ISO 8601 formatted date like all other dates in the normal APIs, and the management console GUI fixed up to know the proper year as well :grinning: In the meantime Im thinking we have to leave this field as a `string` because parsing it is too difficult and not 100% reliable.\r\n\r\n#### Other design decisions\r\n- Specifying \"when\" parameter in EditMaintenanceMode()\r\nThe SetMaintenance method `when` parameter can handle \"any\" string that the [mojombo/chronic](https://github.com/mojombo/chronic) ruby library can parse.  This includes lots of humanised things like \"next tuesday at 5pm\" and \"3pm on the monday before friday in 3 weeks time\".  Luckily for our api case we simply have to take whatever string the user says and pass it through.  But ive also added specific suport to passing in a `DateTimeOffset` object since that is more aligned to our c# nature, and also the special `now` keyword as indicated by the API docs.  To do this i created a `MaintenanceDate` object used as the `When` parameter on the `UpdateMaintenanceRequest` object, and helper functions `MaintenanceDate.FromDateTimeOffset()` `MaintenanceDate.FromChronicString()` and `MaintenanceDate.Now()`\r\n\r\n- MaintenanceModeStatus as an enum\r\nThere appear to be finite values of `off` `on` and `scheduled` so I made this an enum.  The API docs dont definitively state the finite values though...  This isnt too different to other enums we've implemented though\r\n\r\n- Added environment var for management console password for integration tests\r\nadded to the powershell helper script and the `EnterpriseHelper` class, so the integration tests can run\r\n\r\n\r\nThat's about it for now.  Please have a read and provide feedback...  I'm looking at you @shiftkey :grinning: "},"comment":{"url":"https://api.github.com/repos/octokit/octokit.net/issues/comments/239758407","html_url":"https://github.com/octokit/octokit.net/pull/1269#issuecomment-239758407","issue_url":"https://api.github.com/repos/octokit/octokit.net/issues/1269","id":239758407,"user":{"login":"shiftkey","id":359239,"avatar_url":"https://avatars.githubusercontent.com/u/359239?v=3","gravatar_id":"","url":"https://api.github.com/users/shiftkey","html_url":"https://github.com/shiftkey","followers_url":"https://api.github.com/users/shiftkey/followers","following_url":"https://api.github.com/users/shiftkey/following{/other_user}","gists_url":"https://api.github.com/users/shiftkey/gists{/gist_id}","starred_url":"https://api.github.com/users/shiftkey/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shiftkey/subscriptions","organizations_url":"https://api.github.com/users/shiftkey/orgs","repos_url":"https://api.github.com/users/shiftkey/repos","events_url":"https://api.github.com/users/shiftkey/events{/privacy}","received_events_url":"https://api.github.com/users/shiftkey/received_events","type":"User","site_admin":true},"created_at":"2016-08-15T09:02:18Z","updated_at":"2016-08-15T09:02:18Z","body":"@ryangribble apologies again for the delay on this - the approach seems fine, added a few comments\r\n\r\nDigging into the original PR comment:\r\n\r\n#### API Uri's\r\n\r\nI'm fine with isolating that rewriting behaviour as near to the Management Console functionality as possible. We might have a better idea for handling it down the track.\r\n\r\n#### Authenticating\r\n\r\nAlso fine. [Just something we need to live with, it seems.](https://developer.github.com/v3/enterprise/management_console/#authentication)\r\n\r\n> The irksome thing is that the password is essentiall in clear text on the end of the Url but im not sure if that's really any different to it being in the HTTP headers for regular calls anyway...\r\n\r\nIf you're using HTTPS and everything is setup correctly on the server, after the [initial handshake](https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_handshake) between server and client the contents of requests (whether it's the request line or the `Authorization` header) are considered encrypted.\r\n\r\n#### What the heck is up with parameters containing json strings?!!\r\n\r\nGosh that's some beautiful API design :trollface:\r\n\r\n> If anyone is able to show me how to get this working in a more \"normal\" way for octokit where the request object is serialized using the builtin `SimpleJsonSerializer` in the normal pipeline, I would love to see what I missed bercause I feel like I tried so many attempts and still couldnt crack it without going the form encoded route.\r\n\r\nPretty much all of Octokit's serialization has been built off the assumption you're serializing an object to the request body. This is outside that sandbox, so I think we're stuck for now. If SimpleJson is all you need, perhaps we could make the serializer a first class feature on the client, so the code might look like this:\r\n\r\n```csharp\r\npublic Task<MaintenanceModeResponse> EditMaintenanceMode(UpdateMaintenanceRequest maintenance, string managementConsolePassword)\r\n{\r\n    // guard clauses and other setup\r\n \r\n    var payload = \"maintenance=\" + this.Serializer.Serialize(maintenance);\r\n\r\n    return ApiConnection.Post<MaintenanceModeResponse>(endpoint,  payload);\r\n}\r\n```\r\n\r\nI'd need to think about what that might break in terms of testability by baking it into `ApiClient`, and what other scenarios this would help us cover. Also, are there more scenarios where the JSON sent needs to be wildly flexible beyond our simple serializer attributes? Perhaps this won't be worth investing in...\r\n\r\n#### What the heck is up with the time handling?!!\r\n\r\nI'll just leave this here:\r\n\r\n![](https://cloud.githubusercontent.com/assets/359239/17658961/a4d46186-6312-11e6-84de-51f0a830df08.gif)\r\n\r\nOkay, now to actually be constructive.\r\n\r\n - When sending a value as part of the request, it sounds like we should be able to send an ISO 8601 string, right?\r\n - When receiving a value as part of the response, why bother being clever with the parsing? Just serve it up as a string, if the API is going to try and be clever with what it presents.\r\n\r\nI've ask the team about this API and whether others have felt the same pain.\r\n\r\n#### Specifying \"when\" parameter in EditMaintenanceMode()\r\n\r\nThese all seem fair. I'd perhaps drop `FromChronicString` in case it does get obsoleted, but that's a minor feel.\r\n\r\n#### MaintenanceModeStatus as an enum\r\n\r\n#### Added environment var for management console password for integration tests\r\n\r\n:thumbsup: to both\r\n"}}
{"actor":{"display_login":"shiftkey"}}