- Notifications
You must be signed in to change notification settings - Fork928
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 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
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
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.
Don't worry about the number of comments, there's nothing major in them. This looks good in general. 👍🏻
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.
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.
| 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. |
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.
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.
spikecurtisAug 23, 2022 • 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.
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.
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 == "-" { |
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.
Suggestion (non-blocking): We could consider defaulting to usingstdin when stdin is not a tty (and no--file or--license), this is a pretty common practice in cli programs.
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 picked up-f - fromkubectl, FWIW
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.
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>
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.
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 toHashiCorp's route... it seemsreally simple to me.
| func licenses() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Short: "Add, remove, and list licenses", |
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 description doesn't fit what this presently does, so if we did a release this might confuse people.
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.
That's true, but I am working on those, and the converse risk is forgetting to update this
Fixes#3279
Adds a new
coder licenses addsubcommand.