Merge remote-tracking branch 'origin/main' into jjs/presets

coderd/database/dbauthz/dbauthz_test.go

@@ -860,7 +860,6 @@ func (s *MethodTestSuite) TestOrganization() {
 			rbac.ResourceOrganizationMember.InOrg(o.ID).WithID(u.ID), policy.ActionCreate)
 	}))
 	s.Run("InsertPreset", s.Subtest(func(db database.Store, check *expects) {
-		ctx := context.Background()
 		org := dbgen.Organization(s.T(), db, database.Organization{})
 		user := dbgen.User(s.T(), db, database.User{})
 		template := dbgen.Template(s.T(), db, database.Template{
@@ -887,11 +886,10 @@ func (s *MethodTestSuite) TestOrganization() {
 			JobID:             job.ID,
 		})
 		insertPresetParams := database.InsertPresetParams{
+			ID:                uuid.New(),
 			TemplateVersionID: workspaceBuild.TemplateVersionID,
 			Name:              "test",
 		}
-		_, err := db.InsertPreset(ctx, insertPresetParams)
-		require.NoError(s.T(), err)
 		check.Args(insertPresetParams).Asserts(rbac.ResourceTemplate, policy.ActionUpdate)
 	}))
 	s.Run("InsertPresetParameters", s.Subtest(func(db database.Store, check *expects) {
@@ -931,8 +929,6 @@ func (s *MethodTestSuite) TestOrganization() {
 			Names:                   []string{"test"},
 			Values:                  []string{"test"},
 		}
-		_, err = db.InsertPresetParameters(context.Background(), insertPresetParametersParams)
-		require.NoError(s.T(), err)
 		check.Args(insertPresetParametersParams).Asserts(rbac.ResourceTemplate, policy.ActionUpdate)
 	}))
 	s.Run("DeleteOrganizationMember", s.Subtest(func(db database.Store, check *expects) {
@@ -3821,11 +3817,13 @@ func (s *MethodTestSuite) TestSystemFunctions() {
 			CreatedBy:      user.ID,
 		})
 		preset, err := db.InsertPreset(ctx, database.InsertPresetParams{
+			ID:                uuid.New(),
 			TemplateVersionID: templateVersion.ID,
 			Name:              "test",
 		})
 		require.NoError(s.T(), err)
 		_, err = db.InsertPresetParameters(ctx, database.InsertPresetParametersParams{
+			ID:                      uuid.New(),
 			TemplateVersionPresetID: preset.ID,
 			Names:                   []string{"test"},
 			Values:                  []string{"test"},

coderd/database/dump.sql

@@ -1266,14 +1266,14 @@ COMMENT ON COLUMN template_version_parameters.display_order IS 'Specifies the or
 COMMENT ON COLUMN template_version_parameters.ephemeral IS 'The value of an ephemeral parameter will not be preserved between consecutive workspace builds.';
 
 CREATE TABLE template_version_preset_parameters (
-    id uuid DEFAULT gen_random_uuid() NOT NULL,
+    id uuid NOT NULL,
     template_version_preset_id uuid NOT NULL,
     name text NOT NULL,
     value text NOT NULL
 );
 
 CREATE TABLE template_version_presets (
-    id uuid DEFAULT gen_random_uuid() NOT NULL,
+    id uuid NOT NULL,
     template_version_id uuid NOT NULL,
     name text NOT NULL,
     created_at timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL

coderd/database/migrations/000291_workspace_parameter_presets.up.sql

@@ -1,22 +1,17 @@
--- TODO (sasswart): add IF NOT EXISTS and other clauses to make the migration more robust
 CREATE TABLE template_version_presets
 (
-	id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
+	id UUID PRIMARY KEY NOT NULL,
 	template_version_id UUID NOT NULL,
 	name TEXT NOT NULL,
 	created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT CURRENT_TIMESTAMP,
-	-- TODO (sasswart): Will auditing have any relevance to presets?
 	FOREIGN KEY (template_version_id) REFERENCES template_versions (id) ON DELETE CASCADE
 );
 
 CREATE TABLE template_version_preset_parameters
 (
-	id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
+	id UUID PRIMARY KEY NOT NULL,
 	template_version_preset_id UUID NOT NULL,
 	name TEXT NOT NULL,
-	-- TODO (sasswart): would it be beneficial to allow presets to still offer a choice for values?
-	-- This would allow an operator to avoid having to create many similar templates where only one or
-	-- a few values are different.
 	value TEXT NOT NULL,
 	FOREIGN KEY (template_version_preset_id) REFERENCES template_version_presets (id) ON DELETE CASCADE
 );
@@ -28,9 +23,6 @@ ALTER TABLE workspace_builds
 ADD CONSTRAINT workspace_builds_template_version_preset_id_fkey
 FOREIGN KEY (template_version_preset_id)
 REFERENCES template_version_presets (id)
--- TODO (sasswart): SET NULL might not be the best choice here. The rest of the hierarchy has ON DELETE CASCADE.
--- We don't want CASCADE here, because we don't want to delete the workspace build if the preset is deleted.
--- However, do we want to lose record of the preset id for a workspace build?
 ON DELETE SET NULL;
 
 -- Recreate the view to include the new column.

coderd/database/queries.sql.go

@@ -5494,19 +5494,25 @@ func (q *sqlQuerier) GetPresetsByTemplateVersionID(ctx context.Context, template
 
 const insertPreset = `-- name: InsertPreset :one
 INSERT INTO
-	template_version_presets (template_version_id, name, created_at)
+	template_version_presets (id, template_version_id, name, created_at)
 VALUES
-	($1, $2, $3) RETURNING id, template_version_id, name, created_at
+	($1, $2, $3, $4) RETURNING id, template_version_id, name, created_at
 `
 
 type InsertPresetParams struct {
+	ID                uuid.UUID `db:"id" json:"id"`
 	TemplateVersionID uuid.UUID `db:"template_version_id" json:"template_version_id"`
 	Name              string    `db:"name" json:"name"`
 	CreatedAt         time.Time `db:"created_at" json:"created_at"`
 }
 
 func (q *sqlQuerier) InsertPreset(ctx context.Context, arg InsertPresetParams) (TemplateVersionPreset, error) {
-	row := q.db.QueryRowContext(ctx, insertPreset, arg.TemplateVersionID, arg.Name, arg.CreatedAt)
+	row := q.db.QueryRowContext(ctx, insertPreset,
+		arg.ID,
+		arg.TemplateVersionID,
+		arg.Name,
+		arg.CreatedAt,
+	)
 	var i TemplateVersionPreset
 	err := row.Scan(
 		&i.ID,
@@ -5519,22 +5525,29 @@ func (q *sqlQuerier) InsertPreset(ctx context.Context, arg InsertPresetParams) (
 
 const insertPresetParameters = `-- name: InsertPresetParameters :many
 INSERT INTO
-	template_version_preset_parameters (template_version_preset_id, name, value)
+	template_version_preset_parameters (id, template_version_preset_id, name, value)
 SELECT
 	$1,
-	unnest($2 :: TEXT[]),
-	unnest($3 :: TEXT[])
+	$2,
+	unnest($3 :: TEXT[]),
+	unnest($4 :: TEXT[])
 RETURNING id, template_version_preset_id, name, value
 `
 
 type InsertPresetParametersParams struct {
+	ID                      uuid.UUID `db:"id" json:"id"`
 	TemplateVersionPresetID uuid.UUID `db:"template_version_preset_id" json:"template_version_preset_id"`
 	Names                   []string  `db:"names" json:"names"`
 	Values                  []string  `db:"values" json:"values"`
 }
 
 func (q *sqlQuerier) InsertPresetParameters(ctx context.Context, arg InsertPresetParametersParams) ([]TemplateVersionPresetParameter, error) {
-	rows, err := q.db.QueryContext(ctx, insertPresetParameters, arg.TemplateVersionPresetID, pq.Array(arg.Names), pq.Array(arg.Values))
+	rows, err := q.db.QueryContext(ctx, insertPresetParameters,
+		arg.ID,
+		arg.TemplateVersionPresetID,
+		pq.Array(arg.Names),
+		pq.Array(arg.Values),
+	)
 	if err != nil {
 		return nil, err
 	}

coderd/database/queries/presets.sql

@@ -1,13 +1,14 @@
 -- name: InsertPreset :one
 INSERT INTO
-	template_version_presets (template_version_id, name, created_at)
+	template_version_presets (id, template_version_id, name, created_at)
 VALUES
-	(@template_version_id, @name, @created_at) RETURNING *;
+	(@id, @template_version_id, @name, @created_at) RETURNING *;
 
 -- name: InsertPresetParameters :many
 INSERT INTO
-	template_version_preset_parameters (template_version_preset_id, name, value)
+	template_version_preset_parameters (id, template_version_preset_id, name, value)
 SELECT
+	@id,
 	@template_version_preset_id,
 	unnest(@names :: TEXT[]),
 	unnest(@values :: TEXT[])

coderd/provisionerdserver/provisionerdserver.go

@@ -1830,6 +1830,7 @@ func InsertWorkspacePresetsAndParameters(ctx context.Context, logger slog.Logger
 func InsertWorkspacePresetAndParameters(ctx context.Context, db database.Store, templateVersionID uuid.UUID, protoPreset *sdkproto.Preset, t time.Time) error {
 	err := db.InTx(func(tx database.Store) error {
 		dbPreset, err := tx.InsertPreset(ctx, database.InsertPresetParams{
+			ID:                uuid.New(),
 			TemplateVersionID: templateVersionID,
 			Name:              protoPreset.Name,
 			CreatedAt:         t,
@@ -1845,6 +1846,7 @@ func InsertWorkspacePresetAndParameters(ctx context.Context, db database.Store,
 			presetParameterValues = append(presetParameterValues, parameter.Value)
 		}
 		_, err = tx.InsertPresetParameters(ctx, database.InsertPresetParametersParams{
+			ID:                      uuid.New(),
 			TemplateVersionPresetID: dbPreset.ID,
 			Names:                   presetParameterNames,
 			Values:                  presetParameterValues,

coderd/util/maps/maps.go

@@ -8,9 +8,14 @@ import (
 
 // Subset returns true if all the keys of a are present
 // in b and have the same values.
+// If the corresponding value of a[k] is the zero value in
+// b, Subset will skip comparing that value.
+// This allows checking for the presence of map keys.
 func Subset[T, U comparable](a, b map[T]U) bool {
+	var uz U
 	for ka, va := range a {
-		if vb, ok := b[ka]; !ok || va != vb {
+		ignoreZeroValue := va == uz
+		if vb, ok := b[ka]; !ok || (!ignoreZeroValue && va != vb) {
 			return false
 		}
 	}

coderd/util/maps/maps_test.go

@@ -11,8 +11,9 @@ func TestSubset(t *testing.T) {
 	t.Parallel()
 
 	for idx, tc := range []struct {
-		a        map[string]string
-		b        map[string]string
+		a map[string]string
+		b map[string]string
+		// expected value from Subset
 		expected bool
 	}{
 		{
@@ -50,6 +51,24 @@ func TestSubset(t *testing.T) {
 			b:        map[string]string{"a": "1", "b": "3"},
 			expected: false,
 		},
+		// Zero value
+		{
+			a:        map[string]string{"a": "1", "b": ""},
+			b:        map[string]string{"a": "1", "b": "3"},
+			expected: true,
+		},
+		// Zero value, but the other way round
+		{
+			a:        map[string]string{"a": "1", "b": "3"},
+			b:        map[string]string{"a": "1", "b": ""},
+			expected: false,
+		},
+		// Both zero values
+		{
+			a:        map[string]string{"a": "1", "b": ""},
+			b:        map[string]string{"a": "1", "b": ""},
+			expected: true,
+		},
 	} {
 		tc := tc
 		t.Run("#"+strconv.Itoa(idx), func(t *testing.T) {

coderd/workspaceagents_test.go

@@ -1076,7 +1076,8 @@ func TestWorkspaceAgentContainers(t *testing.T) {
 		pool, err := dockertest.NewPool("")
 		require.NoError(t, err, "Could not connect to docker")
 		testLabels := map[string]string{
-			"com.coder.test": uuid.New().String(),
+			"com.coder.test":  uuid.New().String(),
+			"com.coder.empty": "",
 		}
 		ct, err := pool.RunWithOptions(&dockertest.RunOptions{
 			Repository: "busybox",
@@ -1097,7 +1098,10 @@ func TestWorkspaceAgentContainers(t *testing.T) {
 			Repository: "busybox",
 			Tag:        "latest",
 			Cmd:        []string{"sleep", "infinity"},
-			Labels:     map[string]string{"com.coder.test": "ignoreme"},
+			Labels: map[string]string{
+				"com.coder.test":  "ignoreme",
+				"com.coder.empty": "",
+			},
 		}, func(config *docker.HostConfig) {
 			config.AutoRemove = true
 			config.RestartPolicy = docker.RestartPolicy{Name: "no"}