- Notifications
You must be signed in to change notification settings - Fork928
coder features list CLI command#3533
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>
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
| client := coderdtest.New(t, nil) | ||
| coderdtest.CreateFirstUser(t, client) | ||
| cmd, root := clitest.New(t, "features", "list", "-o", "json") |
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.
Possibly slightly weird is that when you do-o json you get the whole Entitlements struct, but when you do-o table (or default output), you only get the features in the table.
What is missing in the table output are the warnings, but our plan is to print license warnings afterevery CLI command, so when that is done you will still get them in the output.
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.
Just one comment on the API, but LGTM.
codersdk/features.go Outdated
| type EntitlementsRequest struct { | ||
| // placeholder so that we can add request parameters in future | ||
| // without breaking changes to the go API | ||
| } |
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 tempted not to do this unless we have certainty that all parameters would be optional.
Otherwise a syntax error wouldn't occur, but the API request may fail.
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 don't have certainty that all parameters will be optional, but adding a new non-optional parameter is always an API breaking change.
If we formulate things with this placeholder, we at least have a straightforward way to add optional parameters in future without a breaking change. It's true that we could doopts ...EntilementsRequestOptions in future, but that's inconsistent with how other methods work, so I added the placeholder...
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.
Hmm, well, looks like my hand is being forced to what you suggest anyway. Typescript doesn't like the empty struct from our autogenerated code, so I'll just drop this. Not worth fixing for something we're not crazy about in the first place.
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
Fixes#3278