From 68fb74860f9f5747a81c5301f781f0bd147be38f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 May 2025 09:40:08 -0500 Subject: [PATCH 1/7] chore: keep previous workspace build parameters for dynamic params --- coderd/wsbuilder/wsbuilder.go | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 46035f28dda77..4c5117070d4a8 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -623,6 +623,11 @@ func (b *Builder) getParameters() (names, values []string, err error) { return nil, nil, BuildError{http.StatusBadRequest, "Unable to build workspace with unsupported parameters", err} } + lastBuildParameterValues := db2sdk.WorkspaceBuildParameters(lastBuildParameters) + resolver := codersdk.ParameterResolver{ + Rich: lastBuildParameterValues, + } + // Dynamic parameters skip all parameter validation. // Deleting a workspace also should skip parameter validation. // Pass the user's input as is. @@ -632,19 +637,33 @@ func (b *Builder) getParameters() (names, values []string, err error) { // conditional parameter existence, the static frame of reference // is not sufficient. So assume the user is correct, or pull in the // dynamic param code to find the actual parameters. + latestValues := make(map[string]string) + for _, latest := range b.richParameterValues { + latestValues[latest.Name] = latest.Value + } + + // Merge the inputs with values from the previous build. + for _, last := range lastBuildParameterValues { + // TODO: Ideally we use the resolver here and look at parameter + // fields such as 'ephemeral'. This requires loading the terraform + // files. For now, just send the previous inputs as is. + if _, exists := latestValues[last.Name]; exists { + continue + } + names = append(names, last.Name) + values = append(values, last.Value) + } + for _, value := range b.richParameterValues { names = append(names, value.Name) values = append(values, value.Value) } + b.parameterNames = &names b.parameterValues = &values return names, values, nil } - resolver := codersdk.ParameterResolver{ - Rich: db2sdk.WorkspaceBuildParameters(lastBuildParameters), - } - for _, templateVersionParameter := range templateVersionParameters { tvp, err := db2sdk.TemplateVersionParameter(templateVersionParameter) if err != nil { From bb234fd6bf804466ab233d6ad03132e4e279fa38 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 May 2025 10:40:44 -0500 Subject: [PATCH 2/7] test: include unit test to assert parameters --- coderd/parameters_test.go | 90 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 83 insertions(+), 7 deletions(-) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 8edadc9b7e797..1d6eea2a69479 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -15,6 +15,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/pubsub" "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/wsjson" "github.com/coder/coder/v2/provisioner/echo" @@ -210,6 +211,79 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { // test to make it obvious what this test is doing. require.Zero(t, setup.api.FileCache.Count()) }) + + t.Run("RebuildParameters", func(t *testing.T) { + t.Parallel() + + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") + require.NoError(t, err) + + modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) + require.NoError(t, err) + + setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{ + provisionerDaemonVersion: provProto.CurrentVersion.String(), + mainTF: dynamicParametersTerraformSource, + modulesArchive: modulesArchive, + plan: nil, + static: nil, + }) + + ctx := testutil.Context(t, testutil.WaitMedium) + stream := setup.stream + previews := stream.Chan() + + // Should see the output of the module represented + preview := testutil.RequireReceive(ctx, t, previews) + require.Equal(t, -1, preview.ID) + require.Empty(t, preview.Diagnostics) + + require.Len(t, preview.Parameters, 1) + require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) + require.True(t, preview.Parameters[0].Value.Valid) + require.Equal(t, "CL", preview.Parameters[0].Value.Value) + _ = stream.Close(websocket.StatusGoingAway) + + wrk := coderdtest.CreateWorkspace(t, setup.client, setup.template.ID, func(request *codersdk.CreateWorkspaceRequest) { + request.RichParameterValues = []codersdk.WorkspaceBuildParameter{ + { + Name: preview.Parameters[0].Name, + Value: "GO", + }, + } + }) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, setup.client, wrk.LatestBuild.ID) + + params, err := setup.client.WorkspaceBuildParameters(ctx, wrk.LatestBuild.ID) + require.NoError(t, err) + require.Len(t, params, 1) + require.Equal(t, "jetbrains_ide", params[0].Name) + require.Equal(t, "GO", params[0].Value) + + // A helper function to assert params + doTransition := func(t *testing.T, trans codersdk.WorkspaceTransition) { + t.Helper() + + bld, err := setup.client.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: setup.template.ActiveVersionID, + Transition: trans, + EnableDynamicParameters: ptr.Ref(true), + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, setup.client, wrk.LatestBuild.ID) + + latestParams, err := setup.client.WorkspaceBuildParameters(ctx, bld.ID) + require.Len(t, latestParams, 1) + require.Equal(t, "jetbrains_ide", latestParams[0].Name) + require.Equal(t, "GO", latestParams[0].Value) + } + + // Restart the workspace, then delete. Asserting params on all builds. + doTransition(t, codersdk.WorkspaceTransitionStop) + doTransition(t, codersdk.WorkspaceTransitionStart) + doTransition(t, codersdk.WorkspaceTransitionDelete) + + }) } type setupDynamicParamsTestParams struct { @@ -225,9 +299,10 @@ type setupDynamicParamsTestParams struct { } type dynamicParamsTest struct { - client *codersdk.Client - api *coderd.API - stream *wsjson.Stream[codersdk.DynamicParametersResponse, codersdk.DynamicParametersRequest] + client *codersdk.Client + api *coderd.API + stream *wsjson.Stream[codersdk.DynamicParametersResponse, codersdk.DynamicParametersRequest] + template codersdk.Template } func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dynamicParamsTest { @@ -259,7 +334,7 @@ func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dyn version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) - _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) + tpl := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) ctx := testutil.Context(t, testutil.WaitShort) stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) @@ -280,9 +355,10 @@ func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dyn }) return dynamicParamsTest{ - client: ownerClient, - stream: stream, - api: api, + client: ownerClient, + api: api, + stream: stream, + template: tpl, } } From 7d84b36bc33eb5f8690217db13978dff5804373a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 May 2025 10:43:20 -0500 Subject: [PATCH 3/7] fmt --- coderd/parameters_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 1d6eea2a69479..96b21cb74f889 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -282,7 +282,6 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { doTransition(t, codersdk.WorkspaceTransitionStop) doTransition(t, codersdk.WorkspaceTransitionStart) doTransition(t, codersdk.WorkspaceTransitionDelete) - }) } From 9299c14a584ec783ba3a148a2e26a336c0acd1f3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 May 2025 10:51:19 -0500 Subject: [PATCH 4/7] overwrite parameter values --- coderd/parameters_test.go | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 96b21cb74f889..139c4189893c2 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -3,6 +3,7 @@ package coderd_test import ( "context" "os" + "slices" "testing" "github.com/google/uuid" @@ -264,18 +265,31 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { doTransition := func(t *testing.T, trans codersdk.WorkspaceTransition) { t.Helper() + fooVal := coderdtest.RandomUsername(t) bld, err := setup.client.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: setup.template.ActiveVersionID, - Transition: trans, + TemplateVersionID: setup.template.ActiveVersionID, + Transition: trans, + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + // No validation, so this should work as is. + // Overwrite the value on each transition + {Name: "foo", Value: fooVal}, + }, EnableDynamicParameters: ptr.Ref(true), }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, setup.client, wrk.LatestBuild.ID) latestParams, err := setup.client.WorkspaceBuildParameters(ctx, bld.ID) - require.Len(t, latestParams, 1) - require.Equal(t, "jetbrains_ide", latestParams[0].Name) - require.Equal(t, "GO", latestParams[0].Value) + require.Len(t, latestParams, 2) + idx := slices.IndexFunc(latestParams, func(parameter codersdk.WorkspaceBuildParameter) bool { + return parameter.Name == "jetbrains_ide" + }) + require.Equal(t, "jetbrains_ide", latestParams[idx].Name) + require.Equal(t, "GO", latestParams[idx].Value) + + fooIdx := (idx + 1) % 2 + require.Equal(t, "foo", latestParams[fooIdx].Name) + require.Equal(t, fooVal, latestParams[fooIdx].Value) } // Restart the workspace, then delete. Asserting params on all builds. From 9215b235f227d98bcde198af5329e78df68097cc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 May 2025 10:53:36 -0500 Subject: [PATCH 5/7] add comment --- coderd/wsbuilder/wsbuilder.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 4c5117070d4a8..71598c531ae6c 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -648,6 +648,7 @@ func (b *Builder) getParameters() (names, values []string, err error) { // fields such as 'ephemeral'. This requires loading the terraform // files. For now, just send the previous inputs as is. if _, exists := latestValues[last.Name]; exists { + // latestValues take priority, so skip this previous value. continue } names = append(names, last.Name) From ea7c20f131be56d8999c23ccf73aa5ef85d12bbf Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 May 2025 11:06:54 -0500 Subject: [PATCH 6/7] linting --- coderd/parameters_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 139c4189893c2..8ad7af99ef1da 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -280,6 +280,8 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { coderdtest.AwaitWorkspaceBuildJobCompleted(t, setup.client, wrk.LatestBuild.ID) latestParams, err := setup.client.WorkspaceBuildParameters(ctx, bld.ID) + require.NoError(t, err) + require.Len(t, latestParams, 2) idx := slices.IndexFunc(latestParams, func(parameter codersdk.WorkspaceBuildParameter) bool { return parameter.Name == "jetbrains_ide" From 28d14168945eea3ec7196a968e4f5979d02d65ca Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 May 2025 09:05:26 -0500 Subject: [PATCH 7/7] better assertion in test --- coderd/parameters_test.go | 14 +++----------- coderd/wsbuilder/wsbuilder.go | 2 +- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 8ad7af99ef1da..dcc494504fd38 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -3,7 +3,6 @@ package coderd_test import ( "context" "os" - "slices" "testing" "github.com/google/uuid" @@ -281,17 +280,10 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { latestParams, err := setup.client.WorkspaceBuildParameters(ctx, bld.ID) require.NoError(t, err) - - require.Len(t, latestParams, 2) - idx := slices.IndexFunc(latestParams, func(parameter codersdk.WorkspaceBuildParameter) bool { - return parameter.Name == "jetbrains_ide" + require.ElementsMatch(t, latestParams, []codersdk.WorkspaceBuildParameter{ + {Name: "jetbrains_ide", Value: "GO"}, + {Name: "foo", Value: fooVal}, }) - require.Equal(t, "jetbrains_ide", latestParams[idx].Name) - require.Equal(t, "GO", latestParams[idx].Value) - - fooIdx := (idx + 1) % 2 - require.Equal(t, "foo", latestParams[fooIdx].Name) - require.Equal(t, fooVal, latestParams[fooIdx].Value) } // Restart the workspace, then delete. Asserting params on all builds. diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 71598c531ae6c..bcc2cef40ebdc 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -637,7 +637,7 @@ func (b *Builder) getParameters() (names, values []string, err error) { // conditional parameter existence, the static frame of reference // is not sufficient. So assume the user is correct, or pull in the // dynamic param code to find the actual parameters. - latestValues := make(map[string]string) + latestValues := make(map[string]string, len(b.richParameterValues)) for _, latest := range b.richParameterValues { latestValues[latest.Name] = latest.Value }