Add suspended as option to AdminService.CreateUser()

[ttomsu]

suspended value can be found here: https://docs.github.com/en/enterprise-server@3.11/rest/enterprise-admin/users?apiVersion=2022-11-28#create-a-user

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b61795d) 97.71% compared to head (3e623b8) 97.71%.

❗ Current head 3e623b8 differs from pull request most recent head db9c8bc. Consider uploading reports for the commit db9c8bc to get more accurate results

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.
📢 Have feedback on the report? Share it here.

[gmlewis]

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?

[ttomsu]

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?

[gmlewis]

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"`
}

[ttomsu]

Change implemented. PTAL

[gmlewis]

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.

[ttomsu]

Apologies for the force commit, my local branch got out of whack and I didn't realize it.

[gmlewis]

Thank you, @ttomsu !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

[ttomsu]

@gmlewis Anyone you can ping to get a second set of eyes on?

[gmlewis]

All contributors to this repo are volunteers and I typically try not to ping individuals unless there is an urgent need.

[fchimpan]

Hi, @ttomsu !
Thanks your implementation. LGTM! Great job!

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.

I agree it. It's a good design.

[gmlewis]

Thank you, @fchimpan !
Merging.