Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 23 commits into from
Jul 10, 2023
Merged

feat(coderd): support ephemeral parameters #8367

merged 23 commits into from
Jul 10, 2023

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Jul 7, 2023 •
edited
Loading

Related: #6828

This PR enables support for ephemeral parameters 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 in terraform-provider-coder).

data "coder_parameter" "force-rebuild" {
  name         = "force-rebuild"
  type           = "bool"
  display_name = "Force rebuild"
  description  = "Rebuild the project code"
  default        = "false"
  mutable      = true
  ephemeral   = true
}

Follow-up issues/PRs:

  • CLI support for ephemeral parameters
  • Adjust site to support ephemeral parameters

@mtojek mtojek self-assigned this Jul 7, 2023
@mtojek mtojek requested review from johnstcn and mafredri July 10, 2023 10:50
@mtojek mtojek marked this pull request as ready for review July 10, 2023 10:50
johnstcn
johnstcn reviewed Jul 10, 2023
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a 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 in go.mod and go.sum. As far as I can tell, this shouldn't require any dependency changes?

codersdk/richparameters_test.go
Comment on lines +335 to +336
Required: true,
Ephemeral: true,
Copy link
Member

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
Comment on lines 362 to 366
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
Copy link
Member

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?

Copy link
Member Author

@mtojek mtojek Jul 10, 2023 •
edited
Loading

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 copy go.mod and go.sum from main here, and then run go mod tidy, this is the result.

EDIT:

Actually it looks the main branch requires go mod tidy?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should add tidy as a lint/gen check?

Copy link
Member Author

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.

@mtojek mtojek mentioned this pull request Jul 10, 2023
Merged
mafredri
mafredri approved these changes Jul 10, 2023
View reviewed changes
codersdk/richparameters_test.go
require.Equal(t, "", v)
}

func TestParameterResolver_ValidateResolve_Ephemeral_RequiredButMissing(t *testing.T) {
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not possible as required is a computed property, the opposite of optional, converted here.

@mtojek mtojek merged commit 9f2a931 into main Jul 10, 2023
@mtojek mtojek deleted the 6828-support-ephemeral-parameters branch July 10, 2023 11:44
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

@johnstcn johnstcn johnstcn approved these changes

Assignees

@mtojek mtojek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mtojek @mafredri @johnstcn