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

Enterprise GitHub #248

Closed
wants to merge 19 commits into from
Closed

Conversation

rcaileff
Copy link

@rcaileff rcaileff commented Mar 2, 2016

GitHub Enterprise Support #99
This implementation is working for me. Let me know if there are pieces that need updating that I didn't realize were relevant. :)

rcaileff added 3 commits March 1, 2016 16:31
Added enterprise versions of the GitHubTests.  They're commented out because they fail unless the repo is set to an available enterprise GitHub repo.
Added EnterpriseGitHubHostname, EnterpriseGitHubAPIPath, EnterpriseGitHubAPIClientId, and EnterpriseGitHubAPIClientSecret keys.
Added GitService.EnterpriseGitHub and extended switch statements to handle it.
Added getEnterpriseGitHubParameters.
Replaced hardcoded strings in WorkspaceMetadata with calls to GitService.foo.hostname().
@rcaileff rcaileff mentioned this pull request Mar 2, 2016
3 tasks
@czechboy0
Copy link
Member

Thanks @rcaileff for this work! It's looking great overall, I just need to clear up one thing:

As far as I understand, when you're running an Enterprise server, each server will have different keys/secrets, so if at all possible, I'd like to avoid users using classic OAuth flow (because we'd need to ask users to create an developer application on their GitHub Enterprise server, copy its id/secret into Buildasaur and only then do the actual authentication). I'd much more prefer users use the simple Personal Token flow. Since users will have to do something manually anyway, creating and copying one Personal Token to Buildasaur is easier than copying key/secret and then doing authentication. Plus, we won't have to add UI for it.

Is there any reason to not use Personal Tokens? If you're fine with it, BuildasaurKeys won't need any extra keys inside. Because the server's URL should be parsed out of the project's remote URL (and somehow we should ping the server to recognize it as a valid GitHub Enterprise server as validation). I'll add comments to the places that I believe you don't need to change if you just use a Personal Token.

Does this make sense? If you're not sure about how the authentication works exactly, GitHub has great docs on it (search for Personal Token).

@@ -14,12 +14,14 @@ import Result

public enum GitService: String {
case GitHub = "github"
case EnterpriseGitHub = "enterprisegithub"
Copy link
Member

Choose a reason for hiding this comment

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

With this addition, I think we should refactor GitService to the following

public enum GitService {
    case GitHub
    case EnterpriseGitHub(url: String)
    case BitBucket
}

Moving to an associated value only for the Enterprise case will make things much easier.

@czechboy0
Copy link
Member

Sorry for bombarding you with those comments, I just wanted to make it easier to go and change the things to disallow OAuth for Enterprise, both in code and in UI.

As far as I see it, we'll need to add

  • parsing of server host+API path from a git repo's remote URL
  • pinging the new server to verify it's a valid Enterprise server

Let me know if you have any questions :) Thanks for working on this. I don't have/use Enterprise, so I'm glad more users will be able to take advantage of Buildasaur.

- updated for Xcode7.3b5, thus for new Swift version
@czechboy0
Copy link
Member

Also, please merge from master, I fixed the Keys issue there.

@rcaileff
Copy link
Author

rcaileff commented Mar 7, 2016

Awesome, thanks.

On Sun, Mar 6, 2016 at 12:09 PM, Honza Dvorsky [email protected]
wrote:

Also, please merge from master, I fixed the Keys issue there.


Reply to this email directly or view it on GitHub
#248 (comment).

rcaileff added 5 commits March 7, 2016 12:57
Added enterprise versions of the GitHubTests.  They're commented out because they fail unless the repo is set to an available enterprise GitHub repo.
Added EnterpriseGitHubHostname, EnterpriseGitHubAPIPath, EnterpriseGitHubAPIClientId, and EnterpriseGitHubAPIClientSecret keys.
Added GitService.EnterpriseGitHub and extended switch statements to handle it.
Added getEnterpriseGitHubParameters.
Replaced hardcoded strings in WorkspaceMetadata with calls to GitService.foo.hostname().
@rcaileff
Copy link
Author

rcaileff commented Mar 7, 2016

I rebased from master and now I have a large number of build errors,
including "Use of unresolved identifier 'BuildasaurKeys' ". :(

On Mon, Mar 7, 2016 at 8:35 AM, Caileff, Rachel [email protected] wrote:

Awesome, thanks.

On Sun, Mar 6, 2016 at 12:09 PM, Honza Dvorsky [email protected]
wrote:

Also, please merge from master, I fixed the Keys issue there.


Reply to this email directly or view it on GitHub
#248 (comment)
.

@czechboy0
Copy link
Member

It's probably still cached, try to delete

  • ~/.cocoapods/keys
  • Pods
    and open Keychain Access and search for any keys starting with Cocoapods-Keys, delete those as well
  • delete derived data

Then run pod install again. Should work, works for me both on CI and locally.

@czechboy0
Copy link
Member

I'll close this since you opened up #251.

@czechboy0 czechboy0 closed this Mar 11, 2016
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