-
Notifications
You must be signed in to change notification settings - Fork 887
chore!: allow CreateUser to accept multiple organizations #14383
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
Conversation
79eb9fa
to
c8e4bdc
Compare
In a multi-org deployment, it makes more sense to allow for multiple org memberships to be assigned at create. The legacy param will still be honored.
c8e4bdc
to
a8fcd42
Compare
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.
Unfortunate about the break change, do we have a plan for how to communicate this on release?
if len(req.OrganizationIDs) == 0 { | ||
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ | ||
Message: "No organization specified to place the user as a member of. It is required to specify at least one organization id to place the user in.", | ||
Detail: "required at least 1 value for the array 'organization_ids'", | ||
Validations: []codersdk.ValidationError{ | ||
{ | ||
Field: "organization_ids", | ||
Detail: "Missing values, this cannot be empty", | ||
}, | ||
}, | ||
}) | ||
return | ||
} |
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.
To clarify, when creating a user they must be in at least 1 org, but later on it's possible to remove them and they have 0 orgs. What is the limitation requiring an org on creation? Just more UI work?
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 correct. There is no limitation, just UX work. This was the existing constraint, and I did not want to change it yet. It'll have to change before multi-org goes live though.
I don't have a great idea. From an API view, all will still work. It's a codersdk breaking change. @bpmct do we know how many users are using our sdk? |
@bpmct mentioned the SDK is used externally. Maybe I can make a new sdk function and deprecate the old. So it will work in the interim. I will spend some thought on this before I merge |
* Handle sdk deprecation better by maintaining cli functions
In a multi-org deployment, it makes more sense to allow for multiple
org memberships to be assigned at create. The legacy param will still
be honored via the api.
This is to prepare for organization sync, a feature that requires creating a user with multiple orgs. Organization sync also introduces the possibility of a user having 0 orgs.
Breaking change
codersdk.CreateUserRequest
no longer accepts the fieldOrganizationID
in Go. There is no way to make this backwards compatible in Go, because the previous value would assumeno value == default org
. This would have to be changed to a*uuid.UUID
to detect the zero value vs not setting it. Which would also be a breaking change.Just made a new cli function and type. We should delete the deprecated methods in the future.
From an API POV (eg typescript), this is backwards compatible via the custom UnmarshalJSON function:
coder/codersdk/users.go
Line 139 in 758751e