-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: members create cmd should invite multiple members #103
fix: members create cmd should invite multiple members #103
Conversation
This pull request has been linked to Shortcut Story #238753: update create member cmd to invite multiple members. |
cmd/members/create.go
Outdated
func runCreate(client members.Client) func(*cobra.Command, []string) error { | ||
return func(cmd *cobra.Command, args []string) error { | ||
var data inputData | ||
var data []ldapi.NewMemberForm |
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'd prefer to try to keep the LD API-specific types encapsulated in the client as much as possible to keep a well-defined seam between our system (the CLI) and another system (the API). We get more control since we can't control the other system's types, so if they change we would need to make changes in multiple places besides the API client.
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.
Could this work with []inputData
?
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.
no because then I think we'd end up with an import cycle, since the client's method would need it. I updated to define the input struct in the ldcli/internal/members
package instead.
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.
fwiw we also import the LD stuff in our flags update here so we might wanna update that as well!
bonus: now we can accept additional member fields (e.g. custom role, team keys, first/last name, password)