Add suspended as option to AdminService.CreateUser()#3049
Add suspended as option to AdminService.CreateUser()#3049gmlewis merged 3 commits intogoogle:masterfrom
Conversation
9e98dd9 to
bdc7d68
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3049 +/- ##
==========================================
- Coverage 97.71% 97.71% -0.01%
==========================================
Files 152 152
Lines 13326 13321 -5
==========================================
- Hits 13021 13016 -5
Misses 215 215
Partials 90 90 ☔ View full report in Codecov by Sentry. |
github/admin_users.go
Outdated
| // | ||
| //meta:operation POST /admin/users | ||
| func (s *AdminService) CreateUser(ctx context.Context, login, email string) (*User, *Response, error) { | ||
| func (s *AdminService) CreateUser(ctx context.Context, login, email string, suspended bool) (*User, *Response, error) { |
There was a problem hiding this comment.
From the official docs: https://docs.github.com/en/enterprise-server@3.11/rest/enterprise-admin/users?apiVersion=2022-11-28#create-a-user
I'm noticing that both the email and suspended fields are optional.
Since we are breaking the API with this PR, I'm wondering if it might make more sense to create a new CreateUserOptions struct containing these optional fields? In addition, login is required, so maybe change line 16 above to Login string json:"login"`.
In other words, we would still use the private createUserRequest but we would copy fields in from opts where the new signature would be:
unc (s *AdminService) CreateUser(ctx context.Context, login string, opts *CreateUserOptions) (*User, *Response, error) {If we do this now, then we might have a bit more flexibility in the future to add new fields to the CreateUserOptions struct without breaking the API again.
Thoughts?
There was a problem hiding this comment.
Hi Glenn - I like the idea to break it only once if we have to break it at all.
Taking a look at other examples around the codebase, it looks like one pattern is to pass URL path segments individually, then the whole object that would end up in the request body. Examples
- TeamsService.CreateTeam(ctx context.Context, org string, team NewTeam)
- PullRequestService.Create(ctx context.Context, owner string, repo string, pull *NewPullRequest)
Following this pattern, it seems a more consistent option would be to export the createUserRequest object and pass that as the second param (after ctx).
WDYT?
There was a problem hiding this comment.
Ah, yes, I see what you are saying. Path query parameters should be listed as separate arguments, and then you can have another parameter:
func (s *AdminService) CreateUser(ctx context.Context, req CreateUserRequest) (*User, *Response, error) {where:
// CreateUserRequest represents the fields sent to the `CreateUser` endpoint.
// Note that `Login` is a required field.
type CreateUserRequest struct {
Login string `json:"login"`
Email *string `json:"email,omitempty"`
Suspended *bool `json:"suspended,omitempty"`
}There was a problem hiding this comment.
Change implemented. PTAL
2a84eae to
3e623b8
Compare
|
In the future, please don't use force-push in this repo, as we always squash-and-merge at the end. See CONTRIBUTING.md for more details. Thanks. |
|
Apologies for the force commit, my local branch got out of whack and I didn't realize it. |
|
@gmlewis Anyone you can ping to get a second set of eyes on? |
|
All contributors to this repo are volunteers and I typically try not to ping individuals unless there is an urgent need. |
|
Hi, @ttomsu !
I agree it. It's a good design. |
|
Thank you, @fchimpan ! |
suspendedvalue can be found here: https://docs.github.com/en/enterprise-server@3.11/rest/enterprise-admin/users?apiVersion=2022-11-28#create-a-user