refac(internal/github): single graphql query to fetch all SSH keys at once - #723
refac(internal/github): single graphql query to fetch all SSH keys at once#723gnarlex wants to merge 2 commits into
Conversation
OliverTrautvetter
left a comment
OliverTrautvetter
left a comment
There was a problem hiding this comment.
Looks good in general, some small stuff
| return nil, err | ||
| } | ||
|
|
||
| team := resp.Data.Organization.Team | ||
| for _, node := range team.Members.Nodes { | ||
| keys := make([]string, 0, len(node.PublicKeys.Nodes)) | ||
| for _, k := range node.PublicKeys.Nodes { | ||
| keys = append(keys, k.Key) | ||
| } | ||
| members = append(members, TeamMemberKeys{Login: node.Login, Keys: keys}) | ||
| } | ||
|
|
||
| if !team.Members.PageInfo.HasNextPage { | ||
| break | ||
| } | ||
| cursor := team.Members.PageInfo.EndCursor | ||
| after = &cursor | ||
| } | ||
|
|
||
| return members, nil |
There was a problem hiding this comment.
Doesn't this silently succeed with 0 keys if the team/org is misconfigured? Before it was a hard 404 error, now it would be (nil, nil)
There was a problem hiding this comment.
404 is not what I would expect from a list members endpoint. If the team/org exists, but has no members, it should return an empty list (nil or empty slice usually have the same behavior, but can return an empty slice if that's preferred) and not return an error. If the caller considers the empty list an error, it's the caller's responsibility to flag this to their user (via error or warning log).
Or am I missing an unhandled error somewhere?
There was a problem hiding this comment.
I agree with that, I just wanted to know if that was an intentional change
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
drops previously fetched members. We could return the partial results if an error happens.
If partial results are usable?
There was a problem hiding this comment.
I think returning a partial result risks giving a false sense of "it worked". Returning all or nothing is IMHO safer.
| pageInfo { hasNextPage endCursor } | ||
| nodes { | ||
| login | ||
| publicKeys(first: 20) { nodes { key } } |
There was a problem hiding this comment.
Silently only the first 20. We should at least log that it is truncated and keys not just vanish silently
c159061 to
6d58245
Compare
2 participants
We are making 1 list request to get all members of a team, and 1 get request for each member's ssh key. For large team's, this results in a lot of requests being made, which may quickly contribute to hitting GitHub API rate limits,
Make a single GraphQL query to get all keys at once.