- Notifications
You must be signed in to change notification settings - Fork928
chore: add edit organization role to cli#13365
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
Emyrk commentedMay 24, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. |
6eb1167 to5ac97f8Comparecbb9432 toc9bd206Compare| return cmd | ||
| } | ||
| func interactiveOrgRoleEdit(inv *serpent.Invocation, orgID uuid.UUID, client *codersdk.Client) (*codersdk.Role, error) { |
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 cool
| Long: FormatExamples( | ||
| Example{ | ||
| Description: "Run with an input.json file", | ||
| Command: "coder roles edit --stdin < role.json", |
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 doesn't seem to work with the output oforganization roles show <orgname> -o json.
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 it does not. Since that command outputs a slice[], and this takes a singular role. Maybe I should make it take a slice??
| r.InitClient(client), | ||
| ), | ||
| Handler: func(inv *serpent.Invocation) error { | ||
| ctx := inv.Context() |
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.
Check forcustom-roles experiment upfront. I tried out the editor but forgot to enable the experiment and ended up only running into the feature gate after creating a custom role.
| // Similar hack is applied to Select() | ||
| if flag.Lookup("test.v") != nil { | ||
| return items, nil | ||
| return opts.Defaults, nil | ||
| } | ||
| prompt := &survey.MultiSelect{ |
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.
The prompt doesn't explain the keybindings:
<space>to select<enter>to submit and return to the previous list<arrow keys>to navigate
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.
Yup it does not. I added it as a note in the PR, just made it an issue:#13427
I talked to@mtojek about this, and I also see Kyle made some adjustments. Essentially, we cannot have different survey templates for different questions. They all share the same global. This global has all the things you mentioned and more. I'd like to revert our change as much as possible, but I'll have to probably adjust every instance that we use this and make sure I did not break anything.
Feels excessive for this PR imo.
| } | ||
| var customRole codersdk.Role | ||
| if jsonInput { |
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.
So the idea here is that the role JSON also contains the role name?
That seems counter-intuitive to me. I would expect to specifycoder organization roles create my-role --stdin < my-role.json.
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.
If I include the role name as an argument, then the json and the arg could differ. Feels strange to define the role_name twice.
The json input honestly is not my favorite, but for cli testing trying to drive the interactive feels like a nightmare.
| if dryRun { | ||
| // Do not actually post | ||
| updated = customRole |
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 think we need a way to actually validate the role without applying it.
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.
Fair. Maybe there should be a dry-run flag in the API request to keep the validation logic client side.
| inv.Stdin = bytes.NewBufferString(fmt.Sprintf(`{ | ||
| "name": "new-role", | ||
| "organization_id": "%s", | ||
| "display_name": "", | ||
| "site_permissions": [], | ||
| "organization_permissions": [ | ||
| { | ||
| "resource_type": "workspace", | ||
| "action": "read" | ||
| } | ||
| ], | ||
| "user_permissions": [], | ||
| "assignable": false, | ||
| "built_in": false | ||
| }`, owner.OrganizationID.String())) |
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.
Add test cases for invalid input.
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 think this could possibly be paired (in a separate PR) with some more tests of thepatchOrganizationRole endpoint that just reads JSON from golden files.
A lot of the UX points I want to kick the can down the road. I know this cli command has it's issues, but at present none of this exists anywhere. What I fear will happen is I could spend a day or two making this cli the best experience (if you mess up currently, you restart). But I am not 100% sure if this UX is even correct. When we build out the UI and land that this role builder strategy is correct, then I'm happy to make another pass before we release (which is a long ways away, as orgs have a lot to be done). |
It feels a bit excessive to drive a lot of json input tests via the cli. I'd rather drive these tests against the api directly. The cli test should be around the cli experience more no? Which I understand these tests are lacking because of the interactive mode. Currently this cli command just serves as a means to use these custom roles for demos. We will make iterations in future on how best to actually define and manage roles. The interactive mode would be quite unprofessional for any large scale operation. |
2ca822f toe5082aeCompare973cc2b intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This is the first draft, I will continue to iterate on this. The cli is missing a lot with organizations. Trying to get in all the groundwork to use orgs.
What this does
Adds
coder organization roles editto create new custom roles for a given organization. This is the first draft, it could use more work to be more useful. However, optimizing the UX of this command at this time feels overkill. This gets in initial code to leverage the feature for local testing.Screencast.from.2024-05-30.10-38-49.webm
It can also be scripted with json as input.
$ coder organization roles edit --stdin< roles.jsonFuture work (before release ideally)
surveypackage templates. We manually changed them, omitting some details in the original that are nice to have./tmpfile