Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Copilot endpoints #2973
Add Copilot endpoints #2973
Changes from 6 commits
aaa5b5b
67e0a68
3e32bab
8dddf5f
0864167
9d5be67
3e27021
568617d
19339c2
4f78d68
2c90d0c
e01ef53
99fe9ba
f2d3230
2ab8aca
b0f05fe
63276a8
d3fa200
78f4db0
da71a5a
766902b
76779c0
baf2515
2adca97
b0181bb
c7a9ad7
56a8c95
5390049
9f70f1f
5e63691
cec367c
1080dff
27decbb
f5b837b
f53e74d
0053173
25e042b
6e03d4e
428a0da
36bc1f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is called copilot-organization-details in github's schema, so CopilotOrganizationDetails might be a better name.
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.
Will do 🙂
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.
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.
I'll have to check whether or not we should be omitting empty, and if we do, how we should be handling that. The reason I bring this up is I've noticed in repo rulesets there's a case where
bypass_actors
should be an empty list of pointers instead ofnil
or omitted otherwise you lose the ability to unset actors when updating the ruleset. (I plan to open a PR to address that specific example in the next couple of days btw).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.
CopilotSeatBreakdown
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.
Ditto
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.
Missing godoc-style comment.
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.
Ditto
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.
This might be better named
ListCopilotSeatsResponse
because it's only used as a response.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.
Ditto
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.
It may be ok to use
*User
here. There are a few other places where we use*User
and let users distinguish using theType
field. @gmlewis will know what the preferred method is.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.
I feel like this might be a better option since we're explicitly setting the correct type for the field, but both approaches leave type-checking to the end user.
There are minor differences as far as I'm aware between the types, e.g.
Team
has noLogin
field, but has aName
field, which bothUser
andOrganization
have.Login
is used for the username in the case of a user, whereasName
is used for team name.I'll leave you guys with these thoughts and will be happy to make changes based on what you think.
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.
That's a good point about Team being different. I think this might be the best way to handle it after all.
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.
type should be
*Timestamp
instead that it works in similar way than other time fields.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.
These can be removed and replaced with
[]string
in the methods that take them as an argument. When you do that, please name the variable something liketeamNames
so that it is clear it takes names and not ids.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.
Ditto
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.
These should have "Copilot" in the name to avoid confusion with any other type of seat in the GitHub API.
I also think that because these are used as responses, it might be better to split these into four structs:
AddCopilotTeamsResponse
,RemoveCopilotTeamsResponse
,AddCopilotUsersResponse
andRemoveCopilotUsersResponse
./cc @gmlewis for thoughts
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.
I don't know, its right place to add this comment here, But may be could we have
GetCopilotSeatInfoForUser
to get the info that user already onboarded before assigning the seat.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.
@WillAbides I'll add the "Copilot" bit in the names. About the structs, the "add" and "remove" structs are identical for users and teams, respectively, because the response schemas are identical.
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.
How are we handling pagination here for more than 100 users?
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.
Will be adding support for this, thanks for the callout 🙏
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.
pagination works in current version
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.
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.
Please delete the blank line between the godoc and the method.
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.
The var name should be downcased "seatCancellations"
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.
Will do, my bad there
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.
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.
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.
Downcase seatCancellations
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.
Ditto
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.