Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement GitHub Enterprise License and Organization API's #1073

Merged
merged 11 commits into from
Feb 11, 2016

Conversation

ryangribble
Copy link
Contributor

Status

🚀Ready to merge🚀

Enhance integration test support for GitHub Enterprise

TattsGroup@535709c
Added a new EnterpriseHelper class for creating enterprise clients, using a new set of environment variables for this purpose. Enhanced the powershell script to assist in setting up these GitHub Enterprise integration test variables if desired

Implement Enterprise License Client

https://developer.github.com/v3/enterprise/license/
Fixes #1023
Includes normal and reactive implementations plus unit and integration tests for both

Implement Enterprise Organization Client

https://developer.github.com/v3/enterprise/orgs/
Fixes #1025
Includes normal and reactive implementations plus unit and integration tests for both

Tests

Obviously the integration tests require a GitHub enterprise instance so wont run as part of Octokit builds currently. Integration tests are passing on my GHE instance

- Remove EnterpriseUrl in integration test Helper class, but leave ability to override custom URL (to allow specific use case of targetting regular integration tests at a custom URL)
- Move GitHub Enterprise explicit support to a new integration helper class using new OCTOKIT_GHE_ environment variables for GHE
- Change existing GitHub Enterprise integration tests and EnterpriseTestAttribute to use the new EnterpriseHelper methods
- Enhance configure-intergration-tests.ps1 script to cater for environment variable changes
@ryangribble
Copy link
Contributor Author

I've rebased this now that the AdminStats PR #1049 has been merged so it should be good for review/merge!


namespace Octokit.Tests.Integration
{
public static class EnterpriseHelper
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍 this is a great idea

@shiftkey
Copy link
Member

shiftkey commented Feb 1, 2016

@ryangribble this is so good!

Just a few conceptual questions about the EnterpriseHelper changes, and apologies for introducing the merge conflict on the csproj files!

@ryangribble
Copy link
Contributor Author

Looks like the Travis build failure is unrelated, @shiftkey ?

@ryangribble
Copy link
Contributor Author

Did the travis build automatically retry or did someone kick it off again? Is there any way (apart from pushing another commit) for plebs to get the builds to try again? 😀

@shiftkey
Copy link
Member

shiftkey commented Feb 2, 2016

Did the travis build automatically retry or did someone kick it off again?

Nope, it was me.

Is there any way (apart from pushing another commit) for plebs to get the builds to try again? 😀

I'm afraid not. We're looking into the Mono issues and where they might be hiding, but that's going to take some time. I'll keep an eye on the build state as part of reviewing - if you're happy that the error isn't your fault just comment so I know you've looked at it.

@ryangribble ryangribble mentioned this pull request Feb 2, 2016
7 tasks
@ryangribble
Copy link
Contributor Author

Hey @shiftkey not sure if you may have forgotten about this one, but ive pulled latest master over again and the builds are still passing. Should be ready for a LGTM 👅

string orgName = String.Concat(orgLogin, " Display Name");

var newOrganization = new NewOrganization(orgLogin, EnterpriseHelper.UserName, orgName);
var observable = _github.Enterprise.Organization.Create(newOrganization);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth doing any cleanup here after creating the organization?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course as soon as I realised that I should read the documentation I realise that "there's no endpoint". So that's a pretty good reason to not do that. Nevermind.

@shiftkey
Copy link
Member

I'm gonna take this in without that bit of 💄. Thanks @ryangribble!

shiftkey added a commit that referenced this pull request Feb 11, 2016
Implement GitHub Enterprise License and Organization API's
@shiftkey shiftkey merged commit dc79d93 into octokit:master Feb 11, 2016
@ryangribble ryangribble deleted the enterprise-api branch February 11, 2016 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants