Skip to content

Add support for Github Enterprise #44

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.

Sign up for GitHub

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

Closed
Line-Noise wants to merge 2 commits into from
Closed

Add support for Github Enterprise #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8948a50
Add support for Github Enterprise through a GITHUB_URL environment va…
shane-mcewan Jul 31, 2019
d6df4de
Merge branch 'master' into master
Line-Noise Aug 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ Alternately, you can download via `go get github.com/clever/microplane/cmd`. In

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/`
Copy link
Copy Markdown
Contributor

@nathanleiby nathanleiby Aug 13, 2019

Choose a reason for hiding this comment

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

nit: consistency re: backticks + URLs ... suggest using backticks around https://github.com too.


### GitLab setup

The `GITLAB_API_TOKEN` environment variable must be set for Gitlab. This should be a [GitLab access token](https://gitlab.com/profile/personal_access_tokens)
Expand Down
18 changes: 16 additions & 2 deletions initialize/initialize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import (
"log"
"os"
"sort"
"strings"
"strings"
"net/url"
Copy link
Copy Markdown
Contributor

@nathanleiby nathanleiby Aug 13, 2019

Choose a reason for hiding this comment

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

needs a go fmt

Copy link
Copy Markdown
Author

@Line-Noise Line-Noise Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.


"github.com/google/go-github/github"
gitlab "github.com/xanzy/go-gitlab"
Expand Down Expand Up @@ -128,6 +129,13 @@ func githubSearch(query string) ([]Repo, error) {

client := github.NewClient(tc)

if os.Getenv("GITHUB_URL") != "" {
baseEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL"))
Copy link
Copy Markdown
Contributor

@nathanleiby nathanleiby Aug 13, 2019

Choose a reason for hiding this comment

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

we should return + handle these errors

Copy link
Copy Markdown
Author

@Line-Noise Line-Noise Aug 15, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@nathanleiby nathanleiby Aug 15, 2019

Choose a reason for hiding this comment

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

I like the idea of using NewEnterpriseClient 👍

client.BaseURL = baseEndpoint
uploadEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL") + "upload/")
client.UploadURL = uploadEndpoint
}

opts := &github.SearchOptions{}
allRepos := map[string]*github.Repository{}
numProcessedResults := 0
Expand Down Expand Up @@ -155,12 +163,18 @@ func githubSearch(query string) ([]Repo, error) {
opts.Page = resp.NextPage
}

hostname := "github.com"
if os.Getenv("GITHUB_URL") != "" {
baseEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL"))
hostname = baseEndpoint.Hostname()
}

repos := []Repo{}
for _, r := range allRepos {
repos = append(repos, Repo{
Name: r.GetName(),
Owner: r.Owner.GetLogin(),
CloneURL: fmt.Sprintf("git@github.com:%s", r.GetFullName()),
CloneURL: fmt.Sprintf("git@%s:%s", hostname, r.GetFullName()),
Provider: "github",
Copy link
Copy Markdown
Contributor

@nathanleiby nathanleiby Aug 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

// Output for Initialize
type Output struct {
Version string
Repos []Repo
}

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.

Copy link
Copy Markdown
Author

@Line-Noise Line-Noise Aug 15, 2019

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! I didn't think about what would happen if someone changed GITHUB_URL part way through the process. It should definitely be made idempotent.

})
}
Expand Down
8 changes: 8 additions & 0 deletions merge/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"os"
"time"
"net/url"

"github.com/google/go-github/github"
"golang.org/x/oauth2"
Expand Down Expand Up @@ -51,6 +52,13 @@ func GitHubMerge(ctx context.Context, input Input, repoLimiter *time.Ticker, mer
tc := oauth2.NewClient(ctx, ts)
client := github.NewClient(tc)

if os.Getenv("GITHUB_URL") != "" {
baseEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL"))
client.BaseURL = baseEndpoint
uploadEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL") + "upload/")
client.UploadURL = uploadEndpoint
}

// OK to merge?

// (1) Check if the PR is mergeable
Expand Down
7 changes: 7 additions & 0 deletions push/push.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ func GithubPush(ctx context.Context, input Input, repoLimiter *time.Ticker, push
tc := oauth2.NewClient(ctx, ts)
client := github.NewClient(tc)

if os.Getenv("GITHUB_URL") != "" {
baseEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL"))
client.BaseURL = baseEndpoint
uploadEndpoint, _ := url.Parse(os.Getenv("GITHUB_URL") + "upload/")
client.UploadURL = uploadEndpoint
}
Copy link
Copy Markdown
Contributor

@nathanleiby nathanleiby Aug 13, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

@Line-Noise Line-Noise Aug 15, 2019

Choose a reason for hiding this comment

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

Yes, I was thinking this as I was adding the same code in all these different places.


// Open a pull request, if one doesn't exist already
head := fmt.Sprintf("%s:%s", input.RepoOwner, input.BranchName)
base := "master"
Expand Down