Skip to content

chore: modify parameter dynamic immutability behavior #18583

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

Merged
merged 11 commits into from
Jul 9, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup test to take an opinion
  • Loading branch information
Emyrk committed Jul 7, 2025
commit e03d15f47415a72da14a2d0bf3766ec8ec08e95a
19 changes: 16 additions & 3 deletions coderd/dynamicparameters/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func ResolveParameters(
//
// This is how the form should look to the user on their workspace settings page.
// This is the original form truth that our validations should initially be based on.
output, diags := renderer.Render(ctx, ownerID, values.ValuesMap())
output, diags := renderer.Render(ctx, ownerID, previousValuesMap)
if diags.HasErrors() {
// Top level diagnostics should break the build. Previous values (and new) should
// always be valid. If there is a case where this is not true, then this has to
Expand All @@ -81,6 +81,7 @@ func ResolveParameters(
//
// To enforce these, the user's input values are trimmed based on the
// mutability and ephemeral parameters defined in the template version.
wasImmutable := make(map[string]struct{})
for _, parameter := range output.Parameters {
// Ephemeral parameters should not be taken from the previous build.
// They must always be explicitly set in every build.
Expand All @@ -98,6 +99,7 @@ func ResolveParameters(
//
// We do this so the next form render uses the original immutable value.
if !firstBuild && !parameter.Mutable {
wasImmutable[parameter.Name] = struct{}{}
delete(values, parameter.Name)
prev, ok := previousValuesMap[parameter.Name]
if ok {
Expand All @@ -123,7 +125,12 @@ func ResolveParameters(
for _, parameter := range output.Parameters {
parameterNames[parameter.Name] = struct{}{}

if !firstBuild && !parameter.Mutable {
// Immutability is sourced from the current `mutable` argument, and also the
// previous parameter's `mutable` argument. This means you cannot flip an
// `immutable` parameter to `mutable` in a single build. This is to preserve the
// original mutability of the parameter.
_, wi := wasImmutable[parameter.Name]
if !firstBuild && (!parameter.Mutable || wi) {
originalValue, ok := originalValues[parameter.Name]
// Immutable parameters should not be changed after the first build.
// If the value matches the original value, that is fine.
Expand All @@ -137,13 +144,19 @@ func ResolveParameters(
if parameter.Source != nil {
src = &parameter.Source.HCLBlock().TypeRange
}
errTitle := "Immutable"
// In the strange case someone flips mutability from `true` to `false`.
// Change the error title to indicate that this was previously immutable.
if wi && parameter.Mutable {
errTitle = "Previously immutable"
}

// An immutable parameter was changed, which is not allowed.
// Add a failed diagnostic to the output.
parameterError.Extend(parameter.Name, hcl.Diagnostics{
&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Immutable parameter changed",
Summary: fmt.Sprintf("%s parameter changed", errTitle),
Detail: fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name),
Subject: src,
},
Expand Down
52 changes: 52 additions & 0 deletions coderd/dynamicparameters/resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,58 @@ import (
func TestResolveParameters(t *testing.T) {
t.Parallel()

t.Run("ChangeImmutable", func(t *testing.T) {
t.Parallel()

ctrl := gomock.NewController(t)
render := rendermock.NewMockRenderer(ctrl)

// The first render takes in the existing values
render.EXPECT().
Render(gomock.Any(), gomock.Any(), gomock.Any()).
Return(&preview.Output{
Parameters: []previewtypes.Parameter{
{
ParameterData: previewtypes.ParameterData{
Name: "immutable",
Type: previewtypes.ParameterTypeString,
FormType: provider.ParameterFormTypeInput,
Mutable: false,
DefaultValue: previewtypes.StringLiteral("foo"),
Required: true,
},
Value: previewtypes.StringLiteral("foo"),
Diagnostics: nil,
},
},
}, nil).
Return(&preview.Output{
Parameters: []previewtypes.Parameter{
{
ParameterData: previewtypes.ParameterData{
Name: "immutable",
Type: previewtypes.ParameterTypeString,
FormType: provider.ParameterFormTypeInput,
Mutable: false,
DefaultValue: previewtypes.StringLiteral("foo"),
Required: true,
},
Value: previewtypes.StringLiteral("foo"),
Diagnostics: nil,
},
},
}, nil)

ctx := testutil.Context(t, testutil.WaitShort)
values, err := dynamicparameters.ResolveParameters(ctx, uuid.New(), render, false,
[]database.WorkspaceBuildParameter{}, // No previous values
[]codersdk.WorkspaceBuildParameter{}, // No new build values
[]database.TemplateVersionPresetParameter{}, // No preset values
)
require.NoError(t, err)
require.Equal(t, map[string]string{"immutable": "foo"}, values)
})

t.Run("NewImmutable", func(t *testing.T) {
t.Parallel()

Expand Down
6 changes: 3 additions & 3 deletions enterprise/coderd/dynamicparameters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ func TestDynamicParameterBuild(t *testing.T) {
t.Run("ImmutableChangeValue", func(t *testing.T) {
// Ok this is a weird test to document how things are working.
// What if a parameter flips it's immutability based on a value?
// The current behavior is to source immutability from the previous build.
t.Parallel()

ctx := testutil.Context(t, testutil.WaitShort)
Expand All @@ -378,15 +379,14 @@ func TestDynamicParameterBuild(t *testing.T) {
coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID)

// Try new values
bld, err := templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{
_, err = templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{
Transition: codersdk.WorkspaceTransitionStart,
RichParameterValues: []codersdk.WorkspaceBuildParameter{
{Name: "isimmutable", Value: "false"},
{Name: "immutable", Value: "not-coder"},
},
})
require.NoError(t, err)
coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, bld.ID)
require.ErrorContains(t, err, `Previously immutable parameter changed`)
})
})
}
Expand Down