- Notifications
You must be signed in to change notification settings - Fork1k
feat: addsharing remove command to the CLI#19767
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
mostly looks good! main concern is just that we should not take an--org flag
cli/sharing_test.go Outdated
| ) | ||
| clitest.SetupConfig(t,workspaceOwnerClient,root) | ||
| out:=bytes.NewBuffer(nil) |
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.
nit:
| out:=bytes.NewBuffer(nil) | |
| out:=new(bytes.Buffer) |
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.
Resolved inthis commit
coderd/workspaces.go Outdated
| // `GetGroups` returns all groups in the org if `GroupIds` is empty so we check | ||
| // the length before making the DB call. |
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.
oops! good catch!
are you sure it's just all groups in the org tho? how would it know which org? looking at the query it seems like it would just return every group regardless of org.
| // `GetGroups` returns all groups in the org if `GroupIds` is empty so we check | |
| // the length before making the DB call. | |
| // `GetGroups` returns all groups if `GroupIds` is empty so we check the length first. |
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.
Resolved inthis commit
cli/sharing.go Outdated
| org,err:=orgContext.Selected(inv,client) | ||
| iferr!=nil { | ||
| returnerr | ||
| } |
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 think we should take--org as a flag. we can already uniquely identify the organization just from thecodersdk.Workspace which hasOrganizationID andOrganizationName fields. this just makes it likely that someone will accidentally pass in the wrong org and cause problems. from looking through this file it seems like those are the only twoorg values that get used anyway, so it also saves us a network round trip!
same for the share/add command.
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.
Good point! I didn't realize that the workspaces had that information already
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.
Resolved inthis commit
Uh oh!
There was an error while loading.Please reload this page.
8d5c566 intomainUh oh!
There was an error while loading.Please reload this page.
Closescoder/internal#861