- Notifications
You must be signed in to change notification settings - Fork928
feat(coderd): support ephemeral parameters#8367
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
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.
The change looks good to me, and it appears to open up some very interesting possibilities.
I just have a few nits about naming, but I'm wary about the changes ingo.mod andgo.sum. As far as I can tell, this shouldn't require any dependency changes?
Uh oh!
There was an error while loading.Please reload this page.
| Required: true, | ||
| Ephemeral: true, |
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 is an interesting use-case - a required ephemeral value could be the equivalent of a TOS checkbox when building a workspace e.g. "I solemnly swear to not use this workspace for evil. [x]"
go.mod Outdated
| github.com/bep/godartsass/v2 v2.0.0 // indirect | ||
| github.com/hashicorp/go-plugin v1.4.4 // indirect | ||
| github.com/hashicorp/terraform-registry-address v0.0.0-20220623143253-7d51757b572c // indirect | ||
| github.com/hashicorp/terraform-svchost v0.0.0-20200729002733-f050f53b9734 // indirect | ||
| github.com/oklog/run v1.0.0 // indirect |
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.
🤔 Is this related to your change?
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 concerned too, but if you copygo.mod andgo.sum from main here, and then rungo mod tidy, this is the result.
EDIT:
Actually it looks the main branch requiresgo mod tidy?
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.
Perhaps we should addtidy as a lint/gen check?
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.
Sounds about right, but rather in a separate PR.
| require.Equal(t, "", v) | ||
| } | ||
| func TestParameterResolver_ValidateResolve_Ephemeral_RequiredButMissing(t *testing.T) { |
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.
What about a case where required = true, but there's also a default value set? Is this possible and should we test it?
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.
Uh oh!
There was an error while loading.Please reload this page.
Related:#6828
This PR enables support for
ephemeralparameters on the backend level. The value of an ephemeral parameter is not persisted between workspace builds, and it returns to default. Ephemeral parameters can't be immutable by definition (restricted by the schema interraform-provider-coder).Follow-up issues/PRs:
ephemeralparametersephemeralparameters