- Notifications
You must be signed in to change notification settings - Fork928
feat: implement organization context in the cli#12259
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
d4d20af to1ac16cfComparecli/organization.go Outdated
| Use: "organizations { current }", | ||
| Short: "Organization related commands", | ||
| Aliases: []string{"organization", "org", "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.
Should we instead do something likecoder ctx current?
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'd be interested in discussing this further, ultimately I'd like for us to support config files for the CLI too, you might not want same behavior across all orgs. Having this support those use-cases as well would be nice.
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. This command is marked as hidden, so we can tweak it before we release.
Multi orgs is still a bit out, but this is going to be really helpful in development.
| Use: "organizations { current }", | ||
| Short: "Organization related commands", | ||
| Aliases: []string{"organization", "org", "orgs"}, | ||
| Hidden: true, // Hidden until these commands are complete. |
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 marked this as hidden, I think we can defer the actual command name discussion and get the code in. Don't want to bikeshed and prevent code progress.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| } | ||
| pty.ExpectMatch("context canceled") |
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.
Why this change?
| return codersdk.Organization{},nil | ||
| return codersdk.Organization{},err |
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.
@mafredri The unit test expectingcontext cancelled had to be changed because of this change. (#12259 (comment))
Before, if you failed to fetch the orgs, we'd return a nil uuid. I made this api failure fail the command now. So the unit tests fails differently now.
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 wouldn't expectTestTemplateCreate/WithVariablesFileWithoutRequiredValue to be testing for org failure though 🤔
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 didn't write the test, but there is a comment indicating this cli error is expected. We cancel the context before running the command.
coder/cli/templatecreate_test.go
Lines 238 to 244 in98666fd
| // We expect the cli to return an error, so we have to handle it | |
| // ourselves. | |
| gofunc() { | |
| cancel() | |
| err:=inv.Run() | |
| assert.Error(t,err) | |
| }() |
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 we're testing against a cancelled context, that test is useless. I think it should either be removed or fixed. 😅 It's definitely not testing what the test name is indicating.
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'm not happy with that test that tests for context cancellation, but not letting that block this PR, so approved. 👍🏻
Yea, I'm not 100% sure what the test is intended to do because the prior behavior makes little sense to me as well. |
Uh oh!
There was an error while loading.Please reload this page.
What this does
The cli now uses the config files to store what organization it is currently referencing.
Adds
coder organizations currentto display the currently selected organization.This is a hidden command until we finish all related commands
Discussion
Currently I have this under
coder organizations { current | switch | ... }. Should we name this something else? Likecoder ctx current,coder ctx switch` etc?This could maybe include different deployments and logins if we do
ctx.coder organizations currentBelow is all the format options.