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.

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

Fix graphql client creation when only v3 url is given #103

Merged
merged 4 commits into from
Jan 11, 2021
Merged

Fix graphql client creation when only v3 url is given #103

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e916fc
Fix graphql client creation when only v3 url is given
taylorsilva Jan 7, 2021
0cea10f
Change to HasSuffix
taylorsilva Jan 8, 2021
1bcc6af
Use TrimSuffix
taylorsilva Jan 8, 2021
60fc3a5
Test that graphql api endpoint is generated correctly
taylorsilva Jan 8, 2021
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
4 changes: 2 additions & 2 deletions github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ func NewGitHubClient(source Source) (*GitHubClient, error) {
}

var v4URL string
if strings.Contains(source.GitHubAPIURL, "v3") {
v4URL = strings.Replace(source.GitHubAPIURL, "v3/", "graphql", 1)
if strings.HasSuffix(source.GitHubAPIURL, "/v3/") {
v4URL = source.GitHubAPIURL[:len(source.GitHubAPIURL)-4] + "/graphql"
} else {
v4URL = source.GitHubAPIURL + "graphql"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :(

Copy link
Contributor

Choose a reason for hiding this comment

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

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

source.GitHubAPIURL = server.URL() + "/"

Assert:

github-release-resource/github_test.go

Line 168 in c82c11c

ghttp.VerifyRequest("POST", "/graphql"),

Copy link
Member Author

@taylorsilva taylorsilva Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added tests for it! and ensured they failed with the original code as well.

Expand Down