Skip to content

coder licenses add CLI command #3632

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 5 commits into from
Aug 23, 2022
Merged

coder licenses add CLI command #3632

merged 5 commits into from
Aug 23, 2022

Conversation

spikecurtis
Copy link
Contributor

Fixes #3279

Adds a new coder licenses add subcommand.

Signed-off-by: Spike Curtis <spike@coder.com>
@spikecurtis spikecurtis requested review from kylecarbs and a team August 22, 2022 22:08
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
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.

Don't worry about the number of comments, there's nothing major in them. This looks good in general. 👍🏻

func TestLicensesAddSuccess(t *testing.T) {
t.Parallel()
// We can't check a real license into the git repo, and can't patch out the keys from here,
// so instead we have to fake the HTTP interaction.
Copy link
Member

Choose a reason for hiding this comment

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

So that we can test licenses for real (in CI) should we consider adding a secret in GitHub actions and only test it in CI (or when defined locally?).

I'm not sure how it would work, just throwing it out there.

Copy link
Contributor Author

@spikecurtis spikecurtis Aug 23, 2022

Choose a reason for hiding this comment

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

Yeah, if we want to build e2e tests, we'll need real licenses that we don't check into the repo


if filename != "" && license == "" {
var r io.Reader
if filename == "-" {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion (non-blocking): We could consider defaulting to using stdin when stdin is not a tty (and no --file or --license), this is a pretty common practice in cli programs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I picked up -f - from kubectl, FWIW

Copy link
Member

@mafredri mafredri Aug 23, 2022

Choose a reason for hiding this comment

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

Oh I didn’t mean to insinuate -f - needs to go away, it’s also very unix-y. My suggestion was something of an addition, but feel free to skip.

Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

From our chat earlier today, I worry that by adding the license to the database we introduce a similar problem of consistency, versioning, and general stability in the product.

I think that this can easily be changed though, so I won't prevent this from merging. I'd like if the method to add a license was similar to HashiCorp's route... it seems really simple to me.


func licenses() *cobra.Command {
cmd := &cobra.Command{
Short: "Add, remove, and list licenses",
Copy link
Member

Choose a reason for hiding this comment

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

This description doesn't fit what this presently does, so if we did a release this might confuse people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, but I am working on those, and the converse risk is forgetting to update this

@spikecurtis spikecurtis merged commit 184f062 into main Aug 23, 2022
@spikecurtis spikecurtis deleted the spike/3279_add_license_cli branch August 23, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

POST license file API
4 participants