Skip to content
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

chore: add edit organization role to cli #13365

Merged
merged 9 commits into from
Jun 3, 2024
Merged

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented May 24, 2024

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 edit to 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.json

Future work (before release ideally)

  • Fix survey package templates. We manually changed them, omitting some details in the original that are nice to have.
  • The cli does not prevent assigning permissions you are not allowed to. An error is returned after submit
  • Cli failures require a complete restart.
  • Write failed edits to a /tmp file

Copy link
Member Author

Emyrk commented May 24, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Emyrk and the rest of your teammates on Graphite Graphite

@Emyrk Emyrk changed the title Add edit org role chore: add edit organization role to cli May 24, 2024
@Emyrk Emyrk marked this pull request as ready for review May 24, 2024 20:43
@Emyrk Emyrk marked this pull request as draft May 24, 2024 20:48
@Emyrk Emyrk force-pushed the stevenmasley/patch_org_roles_rebased branch 2 times, most recently from 6eb1167 to 5ac97f8 Compare May 28, 2024 19:18
Base automatically changed from stevenmasley/patch_org_roles_rebased to main May 29, 2024 14:49
@Emyrk Emyrk force-pushed the stevenmasley/path_org_role_cli branch from cbb9432 to c9bd206 Compare May 29, 2024 20:43
@Emyrk Emyrk marked this pull request as ready for review May 30, 2024 15:43
@Emyrk Emyrk requested a review from johnstcn May 30, 2024 18:24
return cmd
}

func interactiveOrgRoleEdit(inv *serpent.Invocation, orgID uuid.UUID, client *codersdk.Client) (*codersdk.Role, error) {
Copy link
Member

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",
Copy link
Member

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 of organization roles show <orgname> -o json.

Copy link
Member Author

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for custom-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{
Copy link
Member

@johnstcn johnstcn May 31, 2024

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

Copy link
Member Author

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 {
Copy link
Member

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 specify coder organization roles create my-role --stdin < my-role.json.

Copy link
Member Author

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.

Comment on lines +202 to +204
if dryRun {
// Do not actually post
updated = customRole
Copy link
Member

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.

Copy link
Member Author

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.

Comment on lines +41 to +55
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()))
Copy link
Member

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.

Copy link
Member

@johnstcn johnstcn left a 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 the patchOrganizationRole endpoint that just reads JSON from golden files.

@Emyrk
Copy link
Member Author

Emyrk commented May 31, 2024

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).

@Emyrk
Copy link
Member Author

Emyrk commented May 31, 2024

I think this could possibly be paired (in a separate PR) with some more tests of the patchOrganizationRole endpoint that just reads JSON from golden files.

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.

@Emyrk Emyrk force-pushed the stevenmasley/path_org_role_cli branch from 2ca822f to e5082ae Compare May 31, 2024 16:46
@Emyrk Emyrk merged commit 973cc2b into main Jun 3, 2024
32 checks passed
@Emyrk Emyrk deleted the stevenmasley/path_org_role_cli branch June 3, 2024 14:34
@github-actions github-actions bot locked and limited conversation to collaborators Jun 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants