Enable asset downloads from GitHub Enterprise #52
Conversation
In Circle, I received the following error message: "Attempted to download from URL other than the expected download URL of https://github.acme.com/api/v3/repos/temp-internal-org/bash-commons/zipball/v0.0.4. Full error: -1 - Get https://github.acme.com/api/v3/repos/temp-internal-org/bash-commons/zipball/v0.0.4: dial tcp: lookup github.acme.com on 127.0.0.11:53: no such host" Whereas locally, I received the same error message but without "on 127.0.0.11:53". This made me realize the test was unnecessarily brittle. This change should achieve the same check w/ less fusiness.
yorinasub17
left a comment
yorinasub17
left a comment
There was a problem hiding this comment.
LGTM. A few potential style improvements, but they are all minor.
file.go
Outdated
| url = fmt.Sprintf("https://"+instance.ApiUrl+"/repos/%s/%s/zipball/%s", gitHubCommit.Repo.Owner, gitHubCommit.Repo.Name, gitRef) | ||
| } else { | ||
| url = fmt.Sprintf("https://api.github.com/repos/%s/%s/zipball/%s", gitHubCommit.Repo.Owner, gitHubCommit.Repo.Name, gitRef) | ||
| } |
There was a problem hiding this comment.
Can this be simplified to:
url := fmt.Sprintf("https://%s/repos/%s/%s/zipball/%s", instance.ApiUrl, gitHubCommit.Repo.Owner, gitHubCommit.Repo.Name, gitRef)
Since in the else clause, we know instance.ApiUrl = api.github.com?
There was a problem hiding this comment.
Oh gosh, you're right. I was too deep into the idiom of the code that was already there, but yep, this is much cleaner! Fixed in 706117f.
file_test.go
Outdated
| if err != nil && strings.Contains(err.Error(), "no such host") { | ||
| if strings.Contains(err.Error(), githubEnterpriseDownloadUrl) { | ||
| t.Logf("Found expected download URL %s. Download itself failed as expected because no GitHub Enterprise instance exists at the given URL.", githubEnterpriseDownloadUrl) | ||
| t.SkipNow() |
There was a problem hiding this comment.
Does it work if we return here to end the execution with a PASS instead of being marked as SKIP?
There was a problem hiding this comment.
I think so. Unfortunately I'm being throttled by GitHub's API so I've committed this for now in 706117f. Let's see how Circle tests look.
|
Tested this out this evening, and looks to fix #48 that I was seeing in our GHE implementation |
|
Ok, great. PR feedback incorporated, tests pass, real-world use case validated. I think this is ready to merge. Releases should auto-populate in a few minutes. |
|
Fixes #48. |
brikis98
left a comment
brikis98
left a comment
There was a problem hiding this comment.
Oh wow, nice work on catching all the missed error handling!
| // TODO: The awkwardness of this test makes it clear that a better structure for this program would be to refactor | ||
| // the downloadGithubZipFile() function to a function called downloadGithubFile() that would accept a URL as a | ||
| // param. We could then test explicitly that the URL is as expected, which would make GitHub Enterprise test cases | ||
| // simpler to handle. |
There was a problem hiding this comment.
You could run a dummy http server as part of the test (see example in Terratest) and use localhost as the URL you pass to fetch...
There was a problem hiding this comment.
That's a good idea, but just too much work for the available time I had to work on this.
Fixes #48. This is a tricky one to test because we don't have a running instance of GitHub Enterprise against which we can run integration tests. So the best we can do is to validate that the URL fetch attempts to download from in the case of GitHub Enterprise is as expected.
I also noticed a few areas where errors were being swallowed so I added some boilerplate error handling code for those cases.
In the end, for the +178 lines of code additions here, the actual bug fix just 4 lines of code.