-
Notifications
You must be signed in to change notification settings - Fork 887
feat(cli): provide parameter values via command line #8898
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
Conversation
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.
Looks good 👍. A few small suggestions and I'm thinking it might be good to have a bit more test coverage of the precedence of parameter/build values. I.e. when they're coming from previous builds, files and flags (and different combinations of). I don't see that as a reason to delay this PR though.
for i, r := range resolved { | ||
if r.Name == name { | ||
resolved[i].Value = value | ||
goto done |
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: We could label the loop instead and use continue Loop
. Continue/break have a simpler flow and are usually considered more idiomatic Go.
(Same with other loops.)
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.
Are you thinking about this form:
next:
for name, value := range pr.richParametersFile {
for i, r := range resolved {
if r.Name == name {
resolved[i].Value = value
continue next
}
}
resolved = append(resolved, codersdk.WorkspaceBuildParameter{
Name: name,
Value: value,
})
}
If so, then it will be a bit different logic - iterate over pr.richParametersFile
again and again.
Alternatively, I can add break/if, but with goto
the code is shorter/simpler.
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 think there's a misunderstanding around how loop labels work. This is kind of like tagging a loop so that continue
knows where to go. See here (only one iteration through the range is performed): https://go.dev/play/p/oAbzFLuh8O5
The reason I say continue/break [Label]
is simpler, is that they don't break control flow, whereas goto
does and can go anywhere in the function.
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.
return xerrors.Errorf("parameter %q is not present in the template", r.Name) | ||
} | ||
|
||
if tvp.Ephemeral && !pr.promptBuildOptions && len(pr.buildOptions) == 0 { |
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 do we check len 0 here? Wouldn't it be possible for some build options to be provided but not this one? Should are check the contents instead?
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 condition only applies to ephemeral/one-time build parameters, hence tvp.Ephemeral
. !pr.promptBuildOptions
means that the user didn't use the flag --build-options
to input manually values for next build options, and len(pr.buildOptions) == 0
means that they didn't provide them via --build-option key=value
.
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 understand the reasoning for the other checks, however my thinking is that, say there are two ephemeral values. The user providers only one of them via --build-option
. Then len(pr.buildOptions) == 1
even though current tvp
wasn't provided. Does that make sense?
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 admit that I don't want to blow up CLI with an excessive number of test cases. Full coverage would require a matrix |
I'm going to merge this PR, and apply fixes in follow ups if necessary. In the worst case, this PR be reverted entirely as it just modifies CLI. |
Fixes: #8782
This PR refactors the parameter-resolving logic in Coder CLI in terms of consistency and cleaner code. Additionally, it enables passing parameter values via command line flags (
--build-option
and--parameter
).Note:
The business logic is updated. I may need to cover the new flags with tests, but the assumption is that existing CLI tests should not change unless there are new issues discovered.