Implement Enterprise Admin Stats API

[ryangribble]

Closes #1021

Added base Enterprise Client
Implemented EnterpriseAdminStats client and response objects
Added Observable implementations
Added unit tests and integration tests
Added new [GitHubEnterpriseTest] marker for integration tests that require GitHub Enterprise, only discoverable when the integration test variable for GHE is setup

All convention tests, unit tests and integration tests are passing.

[ryangribble]

Hey all, im back from holidays - any feedback on this GitHub Enterprise support? I plan to implement more of the open issues for Enterprise API support, but want to make sure im on the right track first :)

[shiftkey]

Nice spot! ❤️

[ryangribble]

Thanks for the feedback, I've fixed up the async stuff and updated to latest master.

Out of interest with octokit.net contributions, is there a preference between rebase on master vs merging master? Similarly is there a preference to squash the commits down once it's ready to go in, or you aren't bothered by the larger number of in progress commits etc?

[shiftkey]

@ryangribble excellent questions!

Out of interest with octokit.net contributions, is there a preference between rebase on master vs merging master?

I'm not fussed by either approach, but merging master does help keep make the code review process easier to follow on my end.

Similarly is there a preference to squash the commits down once it's ready to go in, or you aren't bothered by the larger number of in progress commits etc?

I'm really not a fan of squashing together dozens of commits when it's done - but you're right about those scenarios where people just want to commit stuff before it's ready. I know I should care about ensuring each commit in each PR builds and passes all tests, but I don't. As long as the history is readable and easy to follow, I'm happy.

[shiftkey]

@haacked thoughts on how we could simplify this method?

For reference, here's the signature from the docs:

And it seems like we're stuck with getting a loose dictionary of values irrespective of the endpoint we use.

[ryangribble]

At first I wrote all the code based on the github API example, assuming the admin stats endpoint would always send back the "collection of stats" regardless of what you asked for, and if you only asked for say "repos" then the other items wouldnt be present (and thus would end up null in my c# object).

But when doing the integration testing what i actually found was that only the "All" method returns back the outside collection (ie collection of stats entries), and all of the other "specific" requests only send back their specific json fragment and not the outer collection. The API doc site only has the "All" example and not an example for say "repo" so this was a bit of an unexpected surprise :)

Since I'd already coded it to have an Enum for the type of stats being requested, I took the "easy" way out and just populated the specific node in the AdminStats collection that is returned by the ALL stats call. This meant there is 1 function, 1 return object (with field/s populated as appropriate), and an Enum to specify the type of stats you want.

The alternative was to remove the enum and add specific functions for every type of request.
eg

Task<AdminStats> GetAllStatistics()
Task<AdminStatsRepo> GetRepoStatistics()
Task<AdminStatsPullRequest> GetPullRequestStatistics()

and so on.

As an end user I felt happy enough with the 1 function/1 return class/enum to specify what I want (most people would probably just request ALL stats anyway im assuming). I felt like that was preferable to adding all the methods etc. But a factor in that was the fact I'd already written most of the code that way...

If you think it's more fitting with the rest of octokit.net, let me know and i can change it to the 11 specific functions for each request

[shiftkey]

@ryangribble thanks for the extra context. I'm not feeling strongly either way at this stage, I was just reflecting on how we don't really have a good example of another API that passes in an enum like this.

For most of the existing API endpoints the parameters are bundled up into a single class - the XYZRequest overloads - but that's not really useful here. Whether you go with one method taking an enum of N values, or N methods each representing one endpoint, the only differences beyond the semantics are:

• does one or the other make more sense to the consumer?
• does one or the other make the implementation code easier or harder?

For me, I think it's a little weird to always get an AdminStats response model back, even if I only asked for users. After the first time it failed, and I'd go "ah, I want the Users property" and update the code, but I'd love to 🔥 the friction off completely.

Do you think we'd get reuse from the existing response models you've created if we went down the "one method per endpoint" scenario? It's going to introduce more methods, but I think they'll be a lot more succinct (and simplify the tests a bit) if we can leverage the serialization cheats we've got to just get back the data we need.

[ryangribble]

Do you think we'd get reuse from the existing response models you've created if we went down the "one method per endpoint" scenario? It's going to introduce more methods, but I think they'll be a lot more succinct (and simplify the tests a bit) if we can leverage the serialization cheats we've got to just get back the data we need.

Yes the response models will all be re-usable, you can see in the switch statement I am basically already calling each endpoint and receiving its response object, im just populating the into the "AdminStats" parent collection and returning that. As I think I said the main thing was that I'd already implemented it one way and then found out at integration testing that it behaves a bit differently when requesting specific stats vs all stats, and the api doc example only has one for the "all" case. Writing individual methods is probably the way to go, it's just annoying that that means heaps more unit and integration tests, where the current tests can just foreach through the enum and cover all 11 request types in the one set of tests). Ill stop being lazy and rework things to see how it looks :)

[shiftkey]

@ryangribble all good, and understandable 😁

[shiftkey]

Just bouncing a question off @haacked about simplifying the code to invoke the endpoint, everything else is 💎

[ryangribble]

@shiftkey as discussed I've rewritten this to use individual methods for each admin stats type, removed the enum, added unit and integration tests for all the new methods. Let me know what you think!

FYI I still need to run the integration tests against our enterprise instance at work tomorrow, so DONT MERGE just yet! I'll advise once integration tests are complete.

Given we are going to be adding more Enterprise API calls to Octokit.net, we need to think about whether we can get access to a GitHub Enterprise instance from the automated builds so they can cover off the Enterprise Integration Tests. @haacked ?

On previously merged PR's I had added a [GitHubEnterpriseTest] test attribute, so the tests are only detected when the OCTOKIT_GITHUBENTERPRISEURL environment variable is present, but specifying that also then uses that enterprise instance for the normal integration tests and there's no specific GHE environment vars for user/password/token etc either. If there is desire to get GitHub Enterprise integration tests as part of the automated process (considering we are planning on adding coverage for all the enterprise endpoints, I think this would be a good idea!) then I will do some more work in another PR to create explicit integration helper class for dealing with GHE including all the required stuff (user, password, token, organization etc etc).

[ryangribble]

OK so I went ahead and did the mentioned integration test tidy ups too. I havent added it to this branch/PR yet as I didnt want to complicate the review process of the actual AdminStats API implementation. I will probably push it through in another PR once this one is merged?

Here is the commit though if you're interested TattsGroup@a9421e6

[ryangribble]

All integration tests passed against my enterprise instance so this should be good to merge (from my perspective)

Not sure why the AppVeyor and Travis builds are failing on unit tests... they are all passing for me locally. Any ideas?

[ryangribble]

I think it's something to do with the different overrides of the ApiConnection.Get() method and the extension method Get(Uri) calling the base Get(Uri, null)

Ah never mind, I just realised there are changes on master I havent pulled in yet which sort out the issue with Get(uri) coming through to the ApiConnection as Get(uri, null), and of course my branch works but the builds are building the result of merging this with master...

doh! Stand by... 😀

[ryangribble]

ok after merge the AppVeyor build is ok, but 1 of the travis builds (OSX) failed (but Linux was ok). I cant quite tell what the OSX problem was, seems to have blown up trying to discover tests in Octokit.Tests-Portable

I did add my GitHub Enterprise unit test classes to this project, eventhough the "FixProjects" build command didn't do it itself. Should I not have done that?

[ryangribble]

Looks like that previous OSX build failure was sporadic as they have now passed after i pushed up a couple of unrelated 💄 commits

I think im done with this PR now @shiftkey

[shiftkey]

💄 duplicate sets of braces here

[ryangribble]

ah nice pickup... Just pushed up that fix, hopefully the builds work 😬

[shiftkey]

@ryangribble looking great - just one excess set of braces in the test.

Looks like that previous OSX build failure was sporadic as they have now passed after i pushed up a couple of unrelated 💄 commits

I've opened #1076 to start tracking these more seriously than I previously have.

[shiftkey]