Fix graphql client creation when only v3 url is given #103
Conversation
fixes concourse/concourse#6373 This commit fixes the case where a user only provides this resource with the `github_api_url` and not the `github_v4_api_url`. In this case the code was appending "graphql" to the end of the provided GitHubAPIURL. Seems this only affects github enterprise users depending on how they configured their v3 and v4 URL's. Apparently something like this was happening for enterprise users when the only set the github_api_url: GitHubAPIURL = api.mygit.com/api/v3 would become GitHubV4APIURL = GitHubAPIURL + "graphql" = api.mygit.com/api/v3/graphql when they really wanted: api.mygit.com/api/graphql Another important thing to know is that the Github Client for v3 expects the API URL to end with a "/". The code already ensures this during creation of a v3 client. For graphql it is expected that the v4 API URL does not end in a "/" Signed-off-by: Taylor Silva <tsilva@pivotal.io>
|
@kramerul could you take a quick look to make sure this makes sense. I don't have an enterprise github I can test this with and I want to make sure I understood your use-case properly and that this will actually fix it. Thanks! |
github.go
Outdated
| @@ -84,7 +84,13 @@ func NewGitHubClient(source Source) (*GitHubClient, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| clientV4 = githubv4.NewEnterpriseClient(source.GitHubAPIURL+"graphql", httpClient) | |||
| var v4URL string | |||
| if strings.Contains(source.GitHubAPIURL, "v3") { | |||
There was a problem hiding this comment.
Should't this be
if strings.HasSuffix(source.GitHubAPIURL, "/v3/") {
v4URL = source.GitHubAPIURL[:len(input)-4] + "/graphql"
} else {
return nil, fmt.Errorf(...)
}
There was a problem hiding this comment.
Changed to HasSuffix() 👍
Not sure if it should return an error for the else case. Can you explain why? I don't think it should return an error based on how the public github api is structured. Their v3 api endpoint is simply api.github.com with no /v3/ appended. Not sure if enterprise github api endpoints can/do follow the same structure.
|
In general it looks OK to me. |
Signed-off-by: Taylor Silva <tsilva@pivotal.io>
github.go
Outdated
| if strings.HasSuffix(source.GitHubAPIURL, "/v3/") { | ||
| v4URL = source.GitHubAPIURL[:len(source.GitHubAPIURL)-4] + "/graphql" | ||
| } else { | ||
| v4URL = source.GitHubAPIURL + "graphql" | ||
| } |
There was a problem hiding this comment.
nit, but you could rewrite this as v4URL = strings.TrimSuffix(source.GitHubAPIURL, "/v3/") + "/graphql" if you get rid of the source.GitHubAPIURL += "/" above (which seems unnecessary?)
also, perhaps we could add a test for this?
There was a problem hiding this comment.
First part sounds good, I'll change that.
For the testing, I looked into that before writing the code and found that I couldn't test this part specifically, verifying that the v4 API url is generated correctly.
Ideally I would verify that the githubv4.Client was given the correct URL. I can do this easily with the v3 client: https://github.com/google/go-github/blob/c2c4a6d477016a3e73a63d67b1933fb24f52fd99/github/github.go#L140-L143
But this is not possible to check with the v4 client, it hides the graphql.client which contains the URL we'd want to check: https://github.com/shurcooL/githubv4/blob/234843c633fadff9694210ce7ab97948c9148a14/githubv4.go#L11-L13
Also, there's only one test for this function to see that it returns an error in one case. After learning all that I decided it wasn't worth adding a test :(
There was a problem hiding this comment.
I think it should be possible, though haven't tried - we validate the path that we receive the request at, so we could set up the GitHubAPIURL as http://some-host:1234/v3 and validate it strips the v3 in the request
Setup:
github-release-resource/github_test.go
Line 126 in c82c11c
Assert:
github-release-resource/github_test.go
Line 168 in c82c11c
There was a problem hiding this comment.
Actually I just thought of a way to check that the URL's it uses are correct! I can use the ghttp.Server to see what endpoints it is hitting. I'll see how hard that is to do, probably not very.
EDIT: I wrote this before seeing your reply, it didn't load for whatever reason :P
There was a problem hiding this comment.
Added tests for it! and ensured they failed with the original code as well.
Signed-off-by: Taylor Silva <tsilva@pivotal.io>
based on the given v3 api endpoint Signed-off-by: Taylor Silva <tsilva@pivotal.io>
fixes concourse/concourse#6373
This commit fixes the case where a user only provides this resource with
the
github_api_urland not thegithub_v4_api_url. In this case thecode was appending "graphql" to the end of the provided GitHubAPIURL.
Seems this only affects github enterprise users depending on how they
configured their v3 and v4 URL's. Apparently something like this was
happening for enterprise users when the only set the github_api_url:
GitHubAPIURL = api.mygit.com/api/v3
would become
GitHubV4APIURL = GitHubAPIURL + "graphql" = api.mygit.com/api/v3/graphql
when they really wanted: api.mygit.com/api/graphql
(I actually have no idea what the github enterprise URL's would look like. They must be different from the public github instance where the previous client creation logic would have worked)
Another important thing to know is that the Github Client for v3 expects
the API URL to end with a "/". The code already ensures this during
creation of a v3 client.
For graphql it is expected that the v4 API URL does not end in a "/"