Skip to content
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.

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

Gitlab support fixes #17

Merged
merged 2 commits into from
Jun 8, 2018
Merged

Gitlab support fixes #17

merged 2 commits into from
Jun 8, 2018

Conversation

Benaiah
Copy link
Contributor

@Benaiah Benaiah commented Jun 6, 2018

Fixes several bugs with GitLab support in git-gateway:

  • Adds HEAD requests to the allowed CORS methods.
  • Adds "Private-Token" to the allowed CORS headers.
  • Constructs a URL by setting r.URL.Opaque instead of by setting r.URL.Path to prevent literal %2Fs in URLs, which are required by the GitLab API, from being turned into literal / characters by the Go stdlib. (see this github issue and this commit, noting that git-gateway is running on go1.8 if I understand correctly from the .travis.yml).
  • Removes the Authorization header from GitLab requests, which causes 401s when used with private access tokens (example), and replaces it with the Private-Token header. (The current WIP Netlify UI has a box for a token to be pasted, so I assume this is supposed to be a private access token, not an OAuth token - which would use the Authorization header).
  • Rewrites absolute links in the GitLab Link header to be relative, so they're requested from git-gateway instead of the proxied API.
  • Adds a default value for the GitLab API endpoint.

api/gitlab.go Outdated
finalLinkEntries := []string{}
for _, linkEntry := range linkEntries {
linkAndRel := strings.Split(strings.TrimSpace(linkEntry), ";")
link := proxyAPIURL + strings.TrimPrefix(gitlabLinkRegex.FindStringSubmatch(linkAndRel[0])[1], endpointAPIURL)
Copy link
Member

Choose a reason for hiding this comment

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

you're indexing into matches, are we 100% sure that these regexes will not fail (odd inputs)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added length-checking before match access to ensure that it's working as expected - the link entry is simply returned unchanged if something goes wrong (this is per-link, so the other links in the Link header can still be rewritten if one doesn't match what we expect).

@rybit
Copy link
Member

rybit commented Jun 7, 2018

also if you want to update the Go version, it might be worth it right now.

api/gitlab.go Outdated

func rewriteGitlabLinks(linkHeader, endpointAPIURL, proxyAPIURL string) string {
linkEntries := strings.Split(linkHeader, ",")
finalLinkEntries := []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can save some allocations by creating the slice with fixed capacity:

finalLinkEntries := make([]string, len(linkEntries), len(linkEntries))

@verythorough
Copy link

It looks like this requires a docs update. At this point, I think that means some minor changes to the README and an update to example.env

@Benaiah
Copy link
Contributor Author

Benaiah commented Jun 7, 2018

@rybit @calavera addressed your review comments - could you re-review? @rybit upgrading Go sounds great, but I'm not sure I know the whole process for doing so for this repo.

api/gitlab.go Outdated
linkEntries := strings.Split(linkHeader, ",")
finalLinkEntries := make([]string, len(linkEntries), len(linkEntries))
for _, linkEntry := range linkEntries {
finalLinkEntries = append(finalLinkEntries, rewriteGitlabLinkEntry(linkEntry, endpointAPIURL, proxyAPIURL))
Copy link
Member

Choose a reason for hiding this comment

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

I think that with the pre-allocation this will cause it to have a bunch of blanks. You'll want to use the index.

https://play.golang.org/p/5a5P3ln5jZe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed, thanks for the catch.

Copy link
Member

@rybit rybit left a comment

Choose a reason for hiding this comment

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

lgtm. I'd love to see more tests in this project in general, but not going to force it.

@rybit rybit merged commit 1aff8bf into netlify:master Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants