Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Feb 26, 2024
Merged

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Feb 21, 2024

What this does

The cli now uses the config files to store what organization it is currently referencing.

Adds coder organizations current to 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? Like coder ctx current, coder ctx switch` etc?

This could maybe include different deployments and logins if we do ctx.

coder organizations current

Below is all the format options.

$ go install && coder organizations current                                                                                                                                
Current organization: admin (3736ca3e-840f-4473-88ad-7056f1a9d22a)

$ coder organizations show current --output=json
[
  {
    "id": "3736ca3e-840f-4473-88ad-7056f1a9d22a",
    "name": "admin",
    "created_at": "2024-02-21T23:22:42.667305Z",
    "updated_at": "2024-02-21T23:22:42.667305Z",
    "is_default": true
  }
]

$ coder organizations show current --output=table
ID                                    NAME   DEFAULT  
3736ca3e-840f-4473-88ad-7056f1a9d22a  admin  true   

# coder organizations show current --only-id
3736ca3e-840f-4473-88ad-7056f1a9d22a

@Emyrk Emyrk changed the title feat: begin work to implement switching orgs in the cli feat: implement switching organizations in the CLI Feb 21, 2024
@Emyrk Emyrk changed the title feat: implement switching organizations in the CLI feat: implement organization context in the cli Feb 22, 2024
@Emyrk Emyrk force-pushed the stevenmasley/select-org branch from d4d20af to 1ac16cf Compare February 22, 2024 14:57
Comment on lines 14 to 16
Use: "organizations { current }",
Short: "Organization related commands",
Aliases: []string{"organization", "org", "orgs"},
Copy link
Member Author

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 like coder ctx current?

Copy link
Member

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.

Copy link
Member Author

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.
Copy link
Member Author

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.

@Emyrk Emyrk marked this pull request as ready for review February 22, 2024 17:01
@Emyrk Emyrk requested a review from mafredri February 22, 2024 17:04
@Emyrk Emyrk mentioned this pull request Feb 23, 2024
}
}

pty.ExpectMatch("context canceled")
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Comment on lines -704 to +716
return codersdk.Organization{}, nil
return codersdk.Organization{}, err
Copy link
Member Author

Choose a reason for hiding this comment

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

@mafredri The unit test expecting context 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.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect TestTemplateCreate/WithVariablesFileWithoutRequiredValue to be testing for org failure though 🤔

Copy link
Member Author

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.

// We expect the cli to return an error, so we have to handle it
// ourselves.
go func() {
cancel()
err := inv.Run()
assert.Error(t, err)
}()

Copy link
Member

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.

@Emyrk Emyrk requested a review from mafredri February 26, 2024 15:25
Copy link
Member

@mafredri mafredri left a 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. 👍🏻

@Emyrk
Copy link
Member Author

Emyrk commented Feb 26, 2024

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.

@Emyrk Emyrk merged commit d2998c6 into main Feb 26, 2024
@Emyrk Emyrk deleted the stevenmasley/select-org branch February 26, 2024 16:03
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 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.

2 participants