- Notifications
You must be signed in to change notification settings - Fork928
feat: switch organization context in coder organizations#12265
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 commentedFeb 21, 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. |
3106518 to9a7aba6Compared4d20af to1ac16cfCompare7142630 tob8ba53eCompareThere 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 wouldn't say any of my comments are blockers, so I'm approving this.
This is a set of nice additions to the CLI!
Uh oh!
There was an error while loading.Please reload this page.
| if len(inv.Args) == 0 { | ||
| // Pull switchToOrg from a prompt selector, rather than command line | ||
| // args. | ||
| switchToOrg, err = promptUserSelectOrg(inv, conf, orgs) |
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.
It'd be nice if the currently selected one is highlighted in the list, perhaps? 🤔
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.
It is! But I read it only from the config file. I ignore theCurrentOrganization because that takes into account the-z flag andis_default.
If a user has nothing set, I want the reset option to be selected.
| return org.Name == switchToOrg || org.ID.String() == switchToOrg | ||
| }) | ||
| if index < 0 { | ||
| // Using this error for better error message formatting |
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.
We should really have one for CLI errors too. (PS. There isexitError, but you might still prefer this approach.)
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.
Yea we really should. I did not want to make a new one for this.
exitError is a single line, this error format will display the helper text on a new line.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| // Verify it worked. | ||
| current, err := CurrentOrganization(r, inv, 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.
Do we need to do this, considering we've fetched orgs and are writing the UUID, what could go wrong?
I mostly worry about increased execution time for the CLI command by additional API roundtrip.
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 mainly added it because if you delete your selection, then this finds what the default is.
cli/organization.go Outdated
| defaultOrg = orgs[index].Name | ||
| } | ||
| const deselectOption = "--Deselect--" |
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 could also be[Default] or the default could be highlighted (kylecarbs (Default)). The latter would have a semantic difference, though, as it would never deselect. Not sure we need to teach users the concept of deselecting an org vs using the default which is essentially what we're doing 🤔
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 like[Default] better than--Deselect--. It is an annoying concept to include, but it felt wrong to not add the option of "Idc, let it choose for me".
My fear was someone runs this command to see what it does, then they are locked into a decision and unable to revert.
Uh oh!
There was an error while loading.Please reload this page.
6b3367a to98666fdCompare1ac5298 tob34cdfeCompare
Uh oh!
There was an error while loading.Please reload this page.
Closes#11927:
Adds a
coder org setto switch the org context.coder org set