support github enterprise by allowing custom api and base urls by jniesen · Pull Request #43 · gruntwork-io/fetch

jniesen · 2018-08-09T20:01:26Z

Fetch seems like a much more enjoyable way to fetch release artifacts from GitHub on the command line. More enjoyable then writing a curl script and more reusable.

However, I'm working with repos in both GitHub Enterprise (GHE) and GitHub.com. These changes make it so that I can pass in the URLs (base and api) of the GHE instance that I'm working with and be able to fetch artifacts from there.

I did some basic smoke testing on this. I was able to fetch from GHE. I can add/fix tests if it's required for merge, it just won't be until tomorrow.

brikis98

Thanks for the PR!

This definitely looks like a useful feature to have. Could you please:

Run the existing automated tests to make sure they pass.
Add a new automated test that sets the new base and API URL params. We don't have a GitHub enterprise instance to test with, so I suppose those could just be set to github.com and api.github.com to make sure those params are used correctly.

brikis98 · 2018-08-09T21:37:33Z

-	Token string // The personal access token to access this repo (if it's a private repo)
+	Url     string // The URL of the GitHub repo
+	BaseUrl string // The Base URL of the GitHub Instance
+	ApiUrl  string // The API Url of the GitHub Instance


Are these 3 ever different? Or, more accurately, is it always the case where:

Url = <domain>/<asset_path> BaseUrl = <domain> ApiUrl = <domain>/api/<version>

I'm mainly trying to figure out if we could deduce <domain> automatically from Url without a new parameter and have an optional ApiVersion param that defaults to something reasonable (v1)...

On github.com, the ApiUrl is always api.github.com and the version can be chosen using an HTTP header. For enterprise, it's always baseurl.com/api/v3.

So we probably could use the repo url to do something like:

baseUrl := parseBaseFromRepoUrl(options.RepoUrl) apiUrl := "api.github.com" if baseUrl != "github.com" { apiUrl := baseUrl+"/api/"+options.ApiVersion }

That seems reasonable to me and, if ApiVersion is set to a default of v3, it makes the user experience easier, as things will "just work" with no extra parameters. Might want to add a logging statement in that if-statement to make it explicit a non github.com URL was found. Also, does that if-statement also need to check against www.github.com?

Also, does that if-statement also need to check against www.github.com?

Yes. Good catch. It will either need to be an || or I can use strings.Contains(baseUrl, "github.com")

I'm running the test suite without my changes and getting some failure right away:

% go test # github.com/jniesen/fetch ./github_test.go:159: Fatalf format %s has arg tc.expected of wrong type main.GitHubReleaseApiResponse ./github_test.go:191: Fatalf format %s has arg tc.assetId of wrong type int ./github_test.go:197: Fatalf format %s has arg tc.assetId of wrong type int FAIL github.com/jniesen/fetch [build failed]

I'm going to look into them, but I'm curious if there's something else in play that I'm missing.

% go version go version go1.10.1 linux/amd64

Could I get temporary read access to: https://github.com/gruntwork-io/fetch-test-private. So that some of the other acceptance tests pass?

Ah, I always forget about that. We have to have a private repo for testing, but then, of course, contributors can't access it 😄

Just added you.

…for it

jniesen · 2018-08-10T15:01:15Z

@brikis98 I made the changes you suggested and updated the test suite. This is ready for review.

brikis98

This looks great, thank you. A few minor issues to fix and this is ready to merge.

brikis98 · 2018-08-10T21:42:20Z

  downloading from private GitHub repos. **NOTE:** fetch will also look for this token using the `GITHUB_OAUTH_TOKEN`
  environment variable, which we recommend using instead of the command line option to ensure the token doesn't get
  saved in bash history.
+- `--github-api-version` (**Optional**): The used when fetching an artifact from a GitHub Enterprise instance.


Remove "the" at the beginning of the sentence. Also, please add "Defaults to v3.

brikis98 · 2018-08-10T21:42:52Z

+  testInst := GitHubInstance{
+    BaseUrl: "github.com",
+    ApiUrl: "api.github.com",
+  }


Indentation is wrong?

brikis98 · 2018-08-10T21:44:10Z

-	Token string // The personal access token to access this repo (if it's a private repo)
+	Url     string // The URL of the GitHub repo
+	BaseUrl string // The Base URL of the GitHub Instance
+	ApiUrl  string // The API Url of the GitHub Instance


Ah, I always forget about that. We have to have a private repo for testing, but then, of course, contributors can't access it 😄

Just added you.

brikis98 · 2018-08-10T21:45:05Z

+func ParseUrlIntoGithubInstance(url string, apiv string) (GitHubInstance, *FetchError) {
+  var instance GitHubInstance
+
+  regex, regexErr := regexp.Compile("https?://(?:www\\.)?(.+?\\.com).*")


Couldn't a user have GitHub Enterprise on a different TLD that isn't .com? Perhaps you should use url.Parse and extract hostname from that?

brikis98 · 2018-08-10T21:55:06Z

+  testInst := GitHubInstance{
+    BaseUrl: "github.com",
+    ApiUrl: "api.github.com",
+  }


Indentation? Please check your editor settings.

Yep. I thought I had vim-go installed, but had vim-polyglot instead which wasn't cutting it on it's own.

brikis98 · 2018-08-10T21:55:18Z

 	}
 }

+func TestParseUrlIntoGithubInstance(t *testing.T) {


Nice tests 👍

jniesen · 2018-08-11T00:34:07Z

Updated to address the things brought up in the last round of review. Thank you for adding me to the repo. I was able to get the complete test suite passing.

brikis98

Great, thank you! I'll merge this now and let the tests run. When they pass, I'll issue a new release and paste the link here.

brikis98 · 2018-08-11T10:21:55Z

https://github.com/gruntwork-io/fetch/releases/tag/v0.3.0

brikis98 reviewed Aug 9, 2018

View reviewed changes

Jonathan Niesen added 9 commits August 10, 2018 10:54

support github enterprise by allowing custom api and base urls

a026c31

fix example 7 in readme

a7f5d55

update test messages so that format verbs match arg types

253ddf5

add a type to represent a github instance and track the various urls …

a273c38

…for it

use the repo url to decipher github instance urls

813a512

update FetchTags and ParseUrlIntoGitHubRepo to take a GitHubInstance

0696ca7

add the optional github-api-version flag

1b85ff7

update tests to work with GitHubInstance

5d57f91

update readme with github enterprise docs

d16a431

jniesen force-pushed the master branch from c5d879e to d16a431 Compare August 10, 2018 15:00

brikis98 reviewed Aug 10, 2018

View reviewed changes

Jonathan Niesen added 4 commits August 10, 2018 19:56

rewording github-api-version docs

2552ef0

fix whitespace issues

51acef1

use url.Parse to cover additional use cases; update tests

18c0bca

update failing test messages to match what is being tested

bb810bf

brikis98 approved these changes Aug 11, 2018

View reviewed changes

brikis98 merged commit ad606a2 into gruntwork-io:master Aug 11, 2018

zackproser mentioned this pull request Apr 21, 2021

Add support for Github enterprise instances gruntwork-io/git-xargs#5

Closed

Uh oh!

Conversation

jniesen commented Aug 9, 2018

Uh oh!

brikis98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jniesen Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jniesen Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jniesen commented Aug 10, 2018

Uh oh!

brikis98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jniesen commented Aug 11, 2018

Uh oh!

brikis98 left a comment

Choose a reason for hiding this comment

Uh oh!

brikis98 commented Aug 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jniesen Aug 10, 2018 •

edited

Loading

jniesen Aug 10, 2018 •

edited

Loading