Enterprise GitHub using Personal Tokens #251

rcaileff · 2016-03-09T17:11:30Z

GitHub Enterprise Support #99 version 2.

I haven't updated my team's server to use this version because I don't want to disrupt our existing CI. I did test this out on my laptop as far as the BuildTemplate selection step. I had to use a hardcoded change to a string value in GitServerFactory.swift because of a quirk in our environment; I don't know if other users will have a similar problem.

Update GitServerPublic.swift to call BuildasaurxcodeprojKeys instead.

Add .EnterpriseGitHub to GitService enum. Change GitService to use an associated value for .EnterpriseGitHub's host. Get the enterprise server's host from the git repo in the keychain. Add and use GitService.type() because the new version of GitService breaks things like "==" and using the rawValue as a dictionary key. Assert that OAuth client/secret will not be use for Enterprise GitHub. Fail horribly if someone tries. Add a couple of EnterpriseGitHubTests that are commented out because they fail without a repo to contact. Replaced hardcoded strings in WorkspaceMetadata with calls to GitService.foo.hostname(). Move some ProjectViewController.ConfigEditViewController logic into the switch statement because the code had assumed anything other than .BitBucket was .GitHub. Also switch BuildasaurxcodeprojKeys back to BuildasaurKeys to be consistent with master.

rcaileff · 2016-03-09T17:14:26Z

The "Fix this" commit has been backed out as part of the "Implement" commit. I don't know why Github insists there is a conflict.

czechboy0 · 2016-03-09T17:42:45Z

BuildaGitServer/GitServerPublic.swift

@@ -41,6 +53,7 @@ public enum GitService: String {
    public func authorizeUrl() -> String {
        switch self {
        case .GitHub: return "https://github.com/login/oauth/authorize"
+        case .EnterpriseGitHub: return "https://\(hostname())/login/oauth/authorize"


I think we should have an assert here as well, since we don't support GitHub Enterprise OAuth yet.

czechboy0 · 2016-03-09T17:45:21Z

Can you please merge master into your branch again? At the moment it can't be merged.

As I didn't manage to get a local GitHub Enterprise to run locally (even though I have a license), I'll rely on you testing it before we merge it. I won't have a chance to verify it.

Update GitServerPublic.swift to call BuildasaurxcodeprojKeys instead.

Add .EnterpriseGitHub to GitService enum. Change GitService to use an associated value for .EnterpriseGitHub's host. Get the enterprise server's host from the git repo in the keychain. Add and use GitService.type() because the new version of GitService breaks things like "==" and using the rawValue as a dictionary key. Assert that OAuth client/secret will not be use for Enterprise GitHub. Fail horribly if someone tries. Add a couple of EnterpriseGitHubTests that are commented out because they fail without a repo to contact. Replaced hardcoded strings in WorkspaceMetadata with calls to GitService.foo.hostname(). Move some ProjectViewController.ConfigEditViewController logic into the switch statement because the code had assumed anything other than .BitBucket was .GitHub. Also switch BuildasaurxcodeprojKeys back to BuildasaurKeys to be consistent with master.

rcaileff · 2016-03-09T21:22:07Z

I still get build errors from master, but here's a merged version for you.

rcaileff · 2016-03-10T21:27:29Z

Merged.

On Wed, Mar 9, 2016 at 11:45 AM, Honza Dvorsky notifications@github.com
wrote:

Can you please merge master into your branch again? At the moment it can't
be merged.

I'll fire up a local Enterprise GitHub server now and give it a try,
thanks so much for your work here! 👍

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

czechboy0 · 2016-03-11T21:46:28Z

Buildasaur/Base.lproj/Main.storyboard

    <dependencies>
        <deployment identifier="macosx"/>
-        <plugIn identifier="com.apple.InterfaceBuilder.CocoaPlugin" version="10089"/>
+        <plugIn identifier="com.apple.InterfaceBuilder.CocoaPlugin" version="10109"/>


Can you please revert these changes, since you didn't change anything I'd prefer to not have a diff here.

…rver in ProjectAuthenticator.fromString. Add tests for ProjectAuthenticator.fromString.

fotiDim · 2016-03-17T16:18:26Z

I tried to use @rcaileff 's branch on our GH Enterprise instance and it failed due to the ldap authentication we use. When you point the tool to the GH API URL it always gets redirected to the LDAP authentication HTML page through which you have to go before getting into GH.

This issue would have been avoided if SSH keys (Deploy keys) authentication was used. Since this is a server-server authentication it makes much sense not to use Personal Tokens but Deploy keys instead.

czechboy0 · 2016-03-17T17:51:00Z

BuildaGitServer/Base/Authentication.swift

@@ -69,4 +73,16 @@ extension ProjectAuthenticator: KeychainStringSerializable {
            self.secret
            ].joinWithSeparator(":")
    }
+
+    internal static func createEnterpriseService(host: String) -> GitService? {
+        guard let url = NSURL.init(string: "http://\(host)") else { return nil }


You can just do NSURL(string: ...).

czechboy0 · 2016-03-17T17:59:33Z

@fotiDim Deploy keys are not yet supported, if you'd like to see support for them in Buildasaur, please create a separate ticket. I agree it would be nice to support them :) We just need to approach these changes in as small steps as possible to ensure everything keeps working.

Regarding this PR, @rcaileff do you understand the issue @fotiDim is having? Unfortunately I don't have a Enterprise server I would have access to, so I can't chime in here.

On that note, would either of you be able to create a test repo on your Enterprise server and give me access (if it's accessible from the outside network)? It would greatly help me to get involved with these Enterprise features, being able to test them and develop more features for them.

rcaileff · 2016-03-17T18:06:40Z

I partially understand the problem @fotiDim is having. Buildasaur comments
in a PR to report the test results. When it does so, it comments as the
user whose credentials are tied to the Personal Token. When I initially
set up Buildasaur, the credentials were tied to my personal work account,
so Buildasaur was making comments as me. This was inconvenient for several
reasons. I'm not sure what his proposed solution involves.

@czechboy0 Unfortunately, I can't give you access to the corporate servers.

On Thu, Mar 17, 2016 at 12:59 PM, Honza Dvorsky notifications@github.com
wrote:

@fotiDim https://github.com/fotiDim Deploy keys are not yet supported,
if you'd like to see support for them in Buildasaur, please create a
separate ticket. I agree it would be nice to support them :) We just need
to approach these changes in as small steps as possible to ensure
everything keeps working.

Regarding this PR, @rcaileff https://github.com/rcaileff do you
understand the issue @fotiDim https://github.com/fotiDim is having?
Unfortunately I don't have a Enterprise server I would have access to, so I
can't chime in here.

On that note, would either of you be able to create a test repo on your
Enterprise server and give me access (if it's accessible from the outside
network)? It would greatly help me to get involved with these Enterprise
features, being able to test them and develop more features for them.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#251 (comment)

czechboy0 · 2016-03-17T18:11:18Z

What I did to solve the first (comments coming in under my personal user) is that I created a separate GitHub account (@buildasaur) whose Personal Tokens I use (after adding it as a collaborator), so that the comments are clearly coming from a CI bot. I recommend you do the same, creating a new GitHub account takes a couple of minutes and the workflow is much nicer then.

I understand it's tricky, I was just asking whether there isn't a way to create one test repository to which I'd have access, I didn't want access to the complete server of course. But if it's impossible there's nothing we can do.

rcaileff · 2016-03-18T14:36:02Z

I did eventually get a separate account to tie to the comments, but because
it's an Enterprise GitHub setup I had to go through corporate channels to
get it created.

On Thu, Mar 17, 2016 at 1:11 PM, Honza Dvorsky notifications@github.com
wrote:

What I did to solve the first (comments coming in under my personal user)
is that I created a separate GitHub account (@buildasaur
https://github.com/buildasaur) whose Personal Tokens I use (after
adding it as a collaborator), so that the comments are clearly coming from
a CI bot. I recommend you do the same, creating a new GitHub account takes
a couple of minutes and the workflow is much nicer then.

I understand it's tricky, I was just asking whether there isn't a way to
create one test repository to which I'd have access, I didn't want access
to the complete server of course. But if it's impossible there's nothing we
can do.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#251 (comment)

fotiDim · 2016-03-21T14:33:59Z

@czechboy0 unfortunately I cannot give you access to our GitHub enterprise instance as it is accessible only in our intranet. I will be happy to test things for though so just ping me when you need sth.

I am still working on the SSH authentication issue and I hope I will have fixed it soon.

rcaileff · 2016-05-05T16:26:03Z

@ Navideck-fotis How is the SSH authentication issue going?

fotiDim · 2016-05-06T11:12:24Z

@rcaileff Hi there. Our internal IT department has failed me. They provided no solution. We settled using a more simple workflow without the benefits of Buildasaur. However our GitHub Server is accessible from the internet now so I might be able to setup an account and a repo for you.

czechboy0 · 2016-05-06T11:54:31Z

@Navideck-fotis 🎉

czechboy0 · 2016-05-10T14:28:30Z

@rcaileff It looks like other PRs have made master conflict with your branch. Could you merge master into your branch, resolve conflicts and get this branch up to date? I'll be happy to merge it once we know it's working! 😊

Refactor createEnterpriseService into GitService and use it in WorkspaceMetadata.swift so parse has a basic check for whether the potential enterprise git server exists. Log the error when createEnterpriseService catches. Dry up TODO comments in enterprise github tests. Add tests for createEnterpriseService and make tryEndpoint more robust. Fix EnterpriseGitHubSourceTests.swift tests to match GitHubServerTests.swift changes. Fix a now-broken GitService comparison in a test helper.

rcaileff · 2016-05-17T19:35:48Z

@czechboy0 I've merged in master and made some minor updates. I have not removed the rate-limitation requirement from the server headers or otherwise tried to handle misconfigured servers.

czechboy0 · 2016-05-17T19:40:28Z

Cool, does this work for you locally? Can anyone else please try to run this with a GitHub Enterprise server for a day and report that everything went well please? Unfortunately I can't test it myself :(

But thanks @rcaileff, this is great stuff, once we're sure it all works I'll merge it in 🎉

rcaileff · 2016-05-17T19:42:57Z

@czechboy0 There are a couple of changes I have to make locally because we also have a misconfigured server... but other than that it works for me. If there's someone out there who can test it on a different system, that would be awesome.

rcaileff · 2016-06-17T20:10:24Z

Merged master back into the branch to resolve conflicts so that the PR is once again able to be merged. Have you had any luck finding someone who can test this? It'd be nice to get it merged into master so I can stop fixing conflicts. 😄

czechboy0 · 2016-07-13T09:22:13Z

Sorry about that, unfortunately it's a large enough change that I don't feel comfortable merging it unless someone is able to give it a good test. If I had access to an enterprise server I could do it myself, however that's not the case right now :(

fotiDim · 2016-10-26T07:17:30Z

I will give it another go internally. If there is any update I will let you know.

sambae · 2016-10-26T13:43:14Z

I am also interested in this feature and will do some testing

sambae · 2016-10-27T16:13:34Z

@fotiDim were you able to get past the ldap authentication page issue?

sambae · 2016-11-01T20:28:53Z

So I was able to get up and running with a couple small changes to the code.

fotiDim · 2016-11-01T21:03:26Z

@sambae good to hear that you got it working. We don't use ldap but active directory and with SAML authentication in the front. I am still waiting for feedback internally.

rcaileff added 3 commits February 24, 2016 17:40

Fix this: Use of unresolved identifier 'BuildasaurKeys'

ae175e9

Update GitServerPublic.swift to call BuildasaurxcodeprojKeys instead.

Fix a couple of errors introduced when I removed a local hack

fa8bc25

rcaileff mentioned this pull request Mar 9, 2016

GitHub Enterprise Support #99

Open

3 tasks

czechboy0 reviewed Mar 9, 2016
View reviewed changes

rcaileff added 7 commits March 9, 2016 13:52

Another assert that Enterprise GitHub doesn't do OAuth

ab06ee3

Fix this: Use of unresolved identifier 'BuildasaurKeys'

d3e308a

Update GitServerPublic.swift to call BuildasaurxcodeprojKeys instead.

Fix a couple of errors introduced when I removed a local hack

935956b

Another assert that Enterprise GitHub doesn't do OAuth

bd5fcbd

Merge xcode project.pbxproj

39f9b63

Merge xcode project.pbxproj again

fa03602

czechboy0 reviewed Mar 11, 2016
View reviewed changes

czechboy0 mentioned this pull request Mar 11, 2016

Enterprise GitHub #248

Closed

rcaileff added 2 commits March 14, 2016 12:04

Revert CocoaPlugin version change in Main.storyboard

8470747

Distinguish between an unsupported server and an Enterprise GitHub se…

70ace29

…rver in ProjectAuthenticator.fromString. Add tests for ProjectAuthenticator.fromString.

czechboy0 reviewed Mar 17, 2016
View reviewed changes

Shorten NSURL.init(...) to NSURL(...)

6db5f12

voidrender mentioned this pull request May 6, 2016

Added support for repo URLs prefixed with ssh:// #260

Merged

czechboy0 mentioned this pull request May 10, 2016

Error Disallowed checkout type, the project must be checked out over one of the supported schemes: SSH #263

Closed

rcaileff added 3 commits May 10, 2016 15:20

Merge branch 'master' into enterprise_v2

4fc6295

Fix xcscmblueprint so BuildaKitTests pass

b29ea8f

czechboy0 mentioned this pull request Jun 3, 2016

Failed to access server, error: 404: for GitHub #278

Closed

Merge branch 'master' into enterprise_v2

56e17bd

czechboy0 closed this May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enterprise GitHub using Personal Tokens #251

Enterprise GitHub using Personal Tokens #251

rcaileff commented Mar 9, 2016

rcaileff commented Mar 9, 2016

czechboy0 Mar 9, 2016

czechboy0 commented Mar 9, 2016

rcaileff commented Mar 9, 2016

rcaileff commented Mar 10, 2016

czechboy0 Mar 11, 2016

rcaileff Mar 14, 2016

fotiDim commented Mar 17, 2016

czechboy0 Mar 17, 2016

rcaileff Mar 18, 2016

czechboy0 commented Mar 17, 2016

rcaileff commented Mar 17, 2016

czechboy0 commented Mar 17, 2016

rcaileff commented Mar 18, 2016

fotiDim commented Mar 21, 2016

rcaileff commented May 5, 2016

fotiDim commented May 6, 2016

czechboy0 commented May 6, 2016

czechboy0 commented May 10, 2016

rcaileff commented May 17, 2016

czechboy0 commented May 17, 2016

rcaileff commented May 17, 2016

rcaileff commented Jun 17, 2016

czechboy0 commented Jul 13, 2016

fotiDim commented Oct 26, 2016

sambae commented Oct 26, 2016

sambae commented Oct 27, 2016

sambae commented Nov 1, 2016

fotiDim commented Nov 1, 2016

Enterprise GitHub using Personal Tokens #251

Enterprise GitHub using Personal Tokens #251

Conversation

rcaileff commented Mar 9, 2016

rcaileff commented Mar 9, 2016

czechboy0 Mar 9, 2016

Choose a reason for hiding this comment

czechboy0 commented Mar 9, 2016

rcaileff commented Mar 9, 2016

rcaileff commented Mar 10, 2016

czechboy0 Mar 11, 2016

Choose a reason for hiding this comment

rcaileff Mar 14, 2016

Choose a reason for hiding this comment

fotiDim commented Mar 17, 2016

czechboy0 Mar 17, 2016

Choose a reason for hiding this comment

rcaileff Mar 18, 2016

Choose a reason for hiding this comment

czechboy0 commented Mar 17, 2016

rcaileff commented Mar 17, 2016

czechboy0 commented Mar 17, 2016

rcaileff commented Mar 18, 2016

fotiDim commented Mar 21, 2016

rcaileff commented May 5, 2016

fotiDim commented May 6, 2016

czechboy0 commented May 6, 2016

czechboy0 commented May 10, 2016

rcaileff commented May 17, 2016

czechboy0 commented May 17, 2016

rcaileff commented May 17, 2016

rcaileff commented Jun 17, 2016

czechboy0 commented Jul 13, 2016

fotiDim commented Oct 26, 2016

sambae commented Oct 26, 2016

sambae commented Oct 27, 2016

sambae commented Nov 1, 2016

fotiDim commented Nov 1, 2016