- Notifications
You must be signed in to change notification settings - Fork1k
feat: addsharing add command to the CLI#19576
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
3ff3630 toa88364cCompareLet's add |
sharing share command to the CLIsharing add command to the CLI
rafrdz left a comment
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.
Overall looks good! Just some small requests for some validation of user input.
| } | ||
| for _, user := range users { | ||
| userAndRole := nameRoleRegex.FindStringSubmatch(user) |
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.
In the event that this isnil can we add a check to prevent a panic?
| userAndRole:=nameRoleRegex.FindStringSubmatch(user) | |
| userAndRole:=nameRoleRegex.FindStringSubmatch(user) | |
| ifuserAndRole==nil { | |
| returnxerrors.Errorf("invalid user format %q: must match pattern 'username:role'",user) | |
| } |
| } | ||
| for _, group := range groups { | ||
| groupAndRole := nameRoleRegex.FindStringSubmatch(group) |
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.
Similar validation question touserAndRole here
909acbc intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Adds a
sharing addcommand for sharing Workspaces with other users and groups.The command allows sharing with multiple users, and groups within one command as well as specifying the role (
use, oradmin) defaulting touseif none is specified.In the current implementation when the command completes we show the user the current state of the workspace ACL.
If a user is a part of multiple groups, or the workspace has been individually shared with them they will show up multiple times. Although this is a bit confusing at first glance it's important to be able to tell what the maximum role a user may have, and via what ACL they have it.
One piece of UX to consider is that in order to be able to share a Workspace with a user they must have a role that can read that user. In the tests we give the user the
ScopedRoleOrgAuditorrole.Closescoder/internal#859