-
Notifications
You must be signed in to change notification settings - Fork 763
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
Properly handle the new style of Node IDs in the GitHub GraphQL API #914
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@jcudit can we get some eyes? This bug is popping up for all new repositories while existing seems to work. |
github/util_v4_repository.go
Outdated
// Check if the name is a base 64 encoded node ID | ||
_, err := base64.StdEncoding.DecodeString(name) | ||
if err != nil { | ||
return false, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any backwards compatibility risks with removing this? Aiming to read more on this front, but hesitating to merge this without more clarity around if this will be a breaking change for some.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcudit That's what I wanted to verify with mocking out the Github API in the unit tests I wrote. I saw in the edit history that every version that accepted a node_id had this local check before calling out to the github api. Originally, it was checking the length. It was checking for base64, but I never saw a version without a pre-check, so it wasn't clear if it was originally put in to address an issue, or if the idea was to simply save an api call.
If you can think of any other cases that'd be worth testing for, I'd be more than happy to add them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originally put in to address an issue
Yes, the aim was to allow passing in a repository name to improve the configuration experience. See #593 for details on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c5a7393 suggests we check for both new and old formats. How does everyone feel about this approach? Seems like a 👎🏾 from a performance / rate limiting perspective but 👍🏾 for reliability.
@@ -16,6 +16,12 @@ func getRepositoryID(name string, meta interface{}) (githubv4.ID, error) { | |||
return githubv4.ID(name), nil | |||
} | |||
|
|||
// Interpret `name` as a legacy node ID | |||
exists, _ = repositoryLegacyNodeIDExists(name, meta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason the error is ignored here? This function already returns an error, so perhaps it's best to add a condition like the following:
exists, _ = repositoryLegacyNodeIDExists(name, meta) | |
exists, err = repositoryLegacyNodeIDExists(name, meta) | |
if err != nil { | |
return nil, err | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh, I see, it's to continue to check the non-legacy case. Please disregard me! This looks good.
…ntegrations#914) * Properly handle the new style of Node IDs in the GitHub GraphQL API https://github.blog/2021-02-10-new-global-id-format-coming-to-graphql/ * fixup! maintain compatibility with deprecated Global ID format Co-authored-by: Emma May <[email protected]> Co-authored-by: Jeremy Udit <[email protected]>
Properly handle branches using the new style of Node IDs in the GraphQL API
https://github.blog/2021-02-10-new-global-id-format-coming-to-graphql/
Addresses #908