Skip to content

fix: fix for prebuilds claiming and deletion #17624

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 8 commits into from
May 1, 2025
2 changes: 1 addition & 1 deletion codersdk/richparameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ type ParameterResolver struct {
// resolves the correct value. It returns the value of the parameter, if valid, and an error if invalid.
func (r *ParameterResolver) ValidateResolve(p TemplateVersionParameter, v *WorkspaceBuildParameter) (value string, err error) {
prevV := r.findLastValue(p)
if !p.Mutable && v != nil && prevV != nil {
if !p.Mutable && v != nil && prevV != nil && v.Value != prevV.Value {
return "", xerrors.Errorf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", p.Name)
}
if p.Required && v == nil && prevV == nil {
Expand Down
57 changes: 49 additions & 8 deletions codersdk/richparameters_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package codersdk_test

import (
"fmt"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -121,20 +122,60 @@ func TestParameterResolver_ValidateResolve_NewOverridesOld(t *testing.T) {
func TestParameterResolver_ValidateResolve_Immutable(t *testing.T) {
t.Parallel()
uut := codersdk.ParameterResolver{
Rich: []codersdk.WorkspaceBuildParameter{{Name: "n", Value: "5"}},
Rich: []codersdk.WorkspaceBuildParameter{{Name: "n", Value: "old"}},
}
p := codersdk.TemplateVersionParameter{
Name: "n",
Type: "number",
Type: "string",
Required: true,
Mutable: false,
}
v, err := uut.ValidateResolve(p, &codersdk.WorkspaceBuildParameter{
Name: "n",
Value: "6",
})
require.Error(t, err)
require.Equal(t, "", v)

cases := []struct {
name string
newValue string
expectedErr string
}{
{
name: "mutation",
newValue: "new", // "new" != "old"
expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name),
},
{
// Values are case-sensitive.
name: "case change",
newValue: "Old", // "Old" != "old"
expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name),
},
{
name: "default",
newValue: "", // "" != "old"
expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name),
},
{
name: "no change",
newValue: "old", // "old" == "old"
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

v, err := uut.ValidateResolve(p, &codersdk.WorkspaceBuildParameter{
Name: "n",
Value: tc.newValue,
})

if tc.expectedErr == "" {
require.NoError(t, err)
require.Equal(t, tc.newValue, v)
} else {
require.ErrorContains(t, err, tc.expectedErr)
require.Equal(t, "", v)
}
})
}
}

func TestRichParameterValidation(t *testing.T) {
Expand Down
16 changes: 13 additions & 3 deletions enterprise/coderd/prebuilds/claim_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,8 @@ func TestClaimPrebuild(t *testing.T) {
require.NoError(t, err)

stopBuild, err := userClient.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: version.ID,
Transition: codersdk.WorkspaceTransitionStop,
RichParameterValues: wp,
TemplateVersionID: version.ID,
Transition: codersdk.WorkspaceTransitionStop,
})
require.NoError(t, err)
coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, stopBuild.ID)
Expand Down Expand Up @@ -369,6 +368,17 @@ func templateWithAgentAndPresetsWithPrebuilds(desiredInstances int32) *echo.Resp
},
},
},
// Make sure immutable params don't break claiming logic
Parameters: []*proto.RichParameter{
{
Name: "k1",
Description: "immutable param",
Type: "string",
DefaultValue: "",
Required: false,
Mutable: false,
},
},
Presets: []*proto.Preset{
{
Name: "preset-a",
Expand Down
19 changes: 18 additions & 1 deletion enterprise/coderd/prebuilds/reconcile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,16 @@ func setupTestDBTemplateVersion(
ID: templateID,
ActiveVersionID: templateVersion.ID,
}))
// Make sure immutable params don't break prebuilt workspace deletion logic
dbgen.TemplateVersionParameter(t, db, database.TemplateVersionParameter{
TemplateVersionID: templateVersion.ID,
Name: "test",
Description: "required & immutable param",
Type: "string",
DefaultValue: "",
Required: true,
Mutable: false,
})
return templateVersion.ID
}

Expand Down Expand Up @@ -999,7 +1009,7 @@ func setupTestDBWorkspace(
OrganizationID: orgID,
Error: buildError,
})
dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
workspaceBuild := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
WorkspaceID: workspace.ID,
InitiatorID: initiatorID,
TemplateVersionID: templateVersionID,
Expand All @@ -1008,6 +1018,13 @@ func setupTestDBWorkspace(
Transition: transition,
CreatedAt: clock.Now(),
})
dbgen.WorkspaceBuildParameters(t, db, []database.WorkspaceBuildParameter{
{
WorkspaceBuildID: workspaceBuild.ID,
Name: "test",
Value: "test",
},
})

return workspace
}
Expand Down
Loading