Conversation
nathanleiby
left a comment
nathanleiby
left a comment
There was a problem hiding this comment.
@Line-Noise Thanks very much for your contribution!
I have a few suggestions that I'd love to see before merging this in. If you don't have time to do them, please let me know, and I'll try to get to them soon.
| The `GITHUB_API_TOKEN` environment variable must be set for Github. This should be a [GitHub Token](https://github.com/settings/tokens) with `repo` scope. | ||
|
|
||
| Optionally: The `GITHUB_URL` environment variable can be set to use a Github Enterprise setup, otherwise it will use https://github.com. | ||
| The `GITHUB_URL` **must** be a valid URL for the API endpoint including a trailing slash: e.g. `https://git.yourcompany.com/api/v3/` |
There was a problem hiding this comment.
nit: consistency re: backticks + URLs ... suggest using backticks around https://github.com too.
| "sort" | ||
| "strings" | ||
| "strings" | ||
| "net/url" |
There was a problem hiding this comment.
Oops. That'll teach me to edit stuff directly in GitHub's editor. It converted the tab to spaces when I fixed the merge conflict. I'll fix it up.
| client := github.NewClient(tc) | ||
|
|
||
| if os.Getenv("GITHUB_URL") != "" { | ||
| baseEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL")) |
There was a problem hiding this comment.
we should return + handle these errors
There was a problem hiding this comment.
I basically cribbed the code direct from https://github.com/google/go-github/blob/master/github/github.go#L310
Perhaps we should be using NewEnterpriseClient if GITHUB_URL is defined and let it do the error handling?
There was a problem hiding this comment.
I like the idea of using NewEnterpriseClient 👍
| client.BaseURL = baseEndpoint | ||
| uploadEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL") + "upload/") | ||
| client.UploadURL = uploadEndpoint | ||
| } |
There was a problem hiding this comment.
by "rule of 3" would love if we could refactor this logic (perhaps all of createGithubClient) into a helper fn so it's not repeated throughout
There was a problem hiding this comment.
Yes, I was thinking this as I was adding the same code in all these different places.
| Owner: r.Owner.GetLogin(), | ||
| CloneURL: fmt.Sprintf("git@github.com:%s", r.GetFullName()), | ||
| CloneURL: fmt.Sprintf("git@%s:%s", hostname, r.GetFullName()), | ||
| Provider: "github", |
There was a problem hiding this comment.
Ultimately, I think I would like to refactor this a bit.. No need to do that within this PR but what do you think of the following?
I believe a it's OK to constrain microplane flow to use only one provider (e.g. all repos come from 1 Github enterprise instance). If so, then we can abstract out the idea of a provider a bit more, make it more explicit, and fix edge cases (e.g. currently, it's possible that the env var value could change and the API URL would be different between push and merge!)
In the future, a user might run: mp init --provider github --provider-url <your API URL> (these flags would be optional and have defaults)
Then, we can store the provider and providerURL on initialize.Output
microplane/initialize/initialize.go
Lines 34 to 39 in f9bb2bd
When doing all future steps, we use the stored provider values. e.g. we can make CloneURL() a function that generates a string based on provider values, and we can remove URL parsing logic in later steps.
There was a problem hiding this comment.
This is a great idea! I didn't think about what would happen if someone changed GITHUB_URL part way through the process. It should definitely be made idempotent.
|
I'll fix up a few things based on your comments but I'm not a |
|
Pretty impressive that you jumped right into a new language! |
|
@nathanleiby it would be great if the support for git enterprise can be merged. This tool can be used widely only if supports enterprise |
|
I started to use this and eventually realized, that there is no support for enterprise :-( |
|
@nathanleiby @Line-Noise any chance this work will be picked up again? |
|
@gtorre Thanks for the follow-up! When I took a look awhile back, I wasn't able to make progress because I didn't have a Github enterprise account to test against. It looks like I'm able to make a 14d free trial for Github Enterprise, so I'll try that and get back to you if I'm able to make this work. |
|
Thanks @nathanleiby ! |
|
@Line-Noise thanks for starting this so long ago and apologies we weren't able to make it happen at the time. I'm revisiting this now and have created two new PRs
I'll close this PR for now, given that the same work lives on in those. Thank you again! |
5 participants
This PR adds basic support for Github Enterprise through a
GITHUB_URLenvironment variable.