From 4cf8536496c69bc3eb5d9f0ae355e9ce10c1c4f0 Mon Sep 17 00:00:00 2001 From: David Wahler Date: Wed, 13 Jul 2022 16:25:28 +0000 Subject: [PATCH 1/3] return parameters from Terraform provisioner in sorted order --- provisioner/terraform/parse.go | 19 ++++++++++- provisioner/terraform/parse_test.go | 52 +++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/provisioner/terraform/parse.go b/provisioner/terraform/parse.go index 7713409e11745..de6781d443622 100644 --- a/provisioner/terraform/parse.go +++ b/provisioner/terraform/parse.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "github.com/hashicorp/terraform-config-inspect/tfconfig" @@ -22,8 +23,17 @@ func (*server) Parse(request *proto.Parse_Request, stream proto.DRPCProvisioner_ return xerrors.Errorf("load module: %s", formatDiagnostics(request.Directory, diags)) } - parameters := make([]*proto.ParameterSchema, 0, len(module.Variables)) + // Sort variables by (filename, line) to make the ordering consistent + variables := make([]*tfconfig.Variable, 0, len(module.Variables)) for _, v := range module.Variables { + variables = append(variables, v) + } + sort.Slice(variables, func(i, j int) bool { + return compareSourcePos(variables[i].Pos, variables[j].Pos) + }) + + parameters := make([]*proto.ParameterSchema, 0, len(variables)) + for _, v := range variables { schema, err := convertVariableToParameter(v) if err != nil { return xerrors.Errorf("convert variable %q: %w", v.Name, err) @@ -129,3 +139,10 @@ func formatDiagnostics(baseDir string, diags tfconfig.Diagnostics) string { return spacer + strings.TrimSpace(msgs.String()) } + +func compareSourcePos(x, y tfconfig.SourcePos) bool { + if x.Filename != y.Filename { + return x.Filename < y.Filename + } + return x.Line < y.Line +} diff --git a/provisioner/terraform/parse_test.go b/provisioner/terraform/parse_test.go index 360ce6c627eea..dab994d335b7d 100644 --- a/provisioner/terraform/parse_test.go +++ b/provisioner/terraform/parse_test.go @@ -128,6 +128,58 @@ func TestParse(t *testing.T) { }, ErrorContains: `The ";" character is not valid.`, }, + { + Name: "multiple-variables", + Files: map[string]string{ + "main1.tf": `variable "foo" { } + variable "bar" { }`, + "main2.tf": `variable "baz" { } + variable "quux" { }`, + }, + Response: &proto.Parse_Response{ + Type: &proto.Parse_Response_Complete{ + Complete: &proto.Parse_Complete{ + ParameterSchemas: []*proto.ParameterSchema{ + { + Name: "foo", + RedisplayValue: true, + AllowOverrideSource: true, + Description: "", + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, + }, + { + Name: "bar", + RedisplayValue: true, + AllowOverrideSource: true, + Description: "", + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, + }, + { + Name: "baz", + RedisplayValue: true, + AllowOverrideSource: true, + Description: "", + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, + }, + { + Name: "quux", + RedisplayValue: true, + AllowOverrideSource: true, + Description: "", + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, + }}, + }, + }, + }, + }, } for _, testCase := range testCases { From bce2cf5b21c2c691fe74ef967cc1784928e5eda1 Mon Sep 17 00:00:00 2001 From: David Wahler Date: Wed, 13 Jul 2022 18:41:57 +0000 Subject: [PATCH 2/3] persist parameter indices in database and return them in correct order from API --- coderd/database/databasefake/databasefake.go | 4 +++ coderd/database/dump.sql | 3 +- .../migrations/000029_variable_order.down.sql | 1 + .../migrations/000029_variable_order.up.sql | 15 ++++++++ coderd/database/models.go | 1 + coderd/database/queries.sql.go | 19 ++++++++--- coderd/database/queries/parameterschemas.sql | 10 ++++-- coderd/provisionerdaemons.go | 4 ++- coderd/templateversions_test.go | 34 +++++++++++++------ 9 files changed, 71 insertions(+), 20 deletions(-) create mode 100644 coderd/database/migrations/000029_variable_order.down.sql create mode 100644 coderd/database/migrations/000029_variable_order.up.sql diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 95920d1ac2b72..a0f5c08f9b4b3 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -1027,6 +1027,9 @@ func (q *fakeQuerier) GetParameterSchemasByJobID(_ context.Context, jobID uuid.U if len(parameters) == 0 { return nil, sql.ErrNoRows } + sort.Slice(parameters, func(i, j int) bool { + return parameters[i].Index < parameters[j].Index + }) return parameters, nil } @@ -1555,6 +1558,7 @@ func (q *fakeQuerier) InsertParameterSchema(_ context.Context, arg database.Inse ValidationCondition: arg.ValidationCondition, ValidationTypeSystem: arg.ValidationTypeSystem, ValidationValueType: arg.ValidationValueType, + Index: arg.Index, } q.parameterSchemas = append(q.parameterSchemas, param) return param, nil diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index b9d6b57762609..9c757a29b4362 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -182,7 +182,8 @@ CREATE TABLE parameter_schemas ( validation_error character varying(256) NOT NULL, validation_condition character varying(512) NOT NULL, validation_type_system parameter_type_system NOT NULL, - validation_value_type character varying(64) NOT NULL + validation_value_type character varying(64) NOT NULL, + index integer NOT NULL ); CREATE TABLE parameter_values ( diff --git a/coderd/database/migrations/000029_variable_order.down.sql b/coderd/database/migrations/000029_variable_order.down.sql new file mode 100644 index 0000000000000..bbb1abbf87344 --- /dev/null +++ b/coderd/database/migrations/000029_variable_order.down.sql @@ -0,0 +1 @@ +ALTER TABLE parameter_schemas DROP COLUMN index; diff --git a/coderd/database/migrations/000029_variable_order.up.sql b/coderd/database/migrations/000029_variable_order.up.sql new file mode 100644 index 0000000000000..2d3735668b4ef --- /dev/null +++ b/coderd/database/migrations/000029_variable_order.up.sql @@ -0,0 +1,15 @@ +-- temporarily create index column as nullable +ALTER TABLE parameter_schemas ADD COLUMN index integer; + +-- initialize ordering for existing parameters +WITH tmp AS ( + SELECT id, row_number() OVER (PARTITION BY job_id ORDER BY name) AS index + FROM parameter_schemas +) +UPDATE parameter_schemas +SET index = tmp.index +FROM tmp +WHERE parameter_schemas.id = tmp.id; + +-- all rows should now be initialized +ALTER TABLE parameter_schemas ALTER COLUMN index SET NOT NULL; diff --git a/coderd/database/models.go b/coderd/database/models.go index 660a7620df454..931f66349d288 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -398,6 +398,7 @@ type ParameterSchema struct { ValidationCondition string `db:"validation_condition" json:"validation_condition"` ValidationTypeSystem ParameterTypeSystem `db:"validation_type_system" json:"validation_type_system"` ValidationValueType string `db:"validation_value_type" json:"validation_value_type"` + Index int32 `db:"index" json:"index"` } type ParameterValue struct { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a85b18bda4c30..8addc7a2bdaab 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -849,11 +849,13 @@ func (q *sqlQuerier) InsertOrganization(ctx context.Context, arg InsertOrganizat const getParameterSchemasByJobID = `-- name: GetParameterSchemasByJobID :many SELECT - id, created_at, job_id, name, description, default_source_scheme, default_source_value, allow_override_source, default_destination_scheme, allow_override_destination, default_refresh, redisplay_value, validation_error, validation_condition, validation_type_system, validation_value_type + id, created_at, job_id, name, description, default_source_scheme, default_source_value, allow_override_source, default_destination_scheme, allow_override_destination, default_refresh, redisplay_value, validation_error, validation_condition, validation_type_system, validation_value_type, index FROM parameter_schemas WHERE job_id = $1 +ORDER BY + index ` func (q *sqlQuerier) GetParameterSchemasByJobID(ctx context.Context, jobID uuid.UUID) ([]ParameterSchema, error) { @@ -882,6 +884,7 @@ func (q *sqlQuerier) GetParameterSchemasByJobID(ctx context.Context, jobID uuid. &i.ValidationCondition, &i.ValidationTypeSystem, &i.ValidationValueType, + &i.Index, ); err != nil { return nil, err } @@ -897,7 +900,7 @@ func (q *sqlQuerier) GetParameterSchemasByJobID(ctx context.Context, jobID uuid. } const getParameterSchemasCreatedAfter = `-- name: GetParameterSchemasCreatedAfter :many -SELECT id, created_at, job_id, name, description, default_source_scheme, default_source_value, allow_override_source, default_destination_scheme, allow_override_destination, default_refresh, redisplay_value, validation_error, validation_condition, validation_type_system, validation_value_type FROM parameter_schemas WHERE created_at > $1 +SELECT id, created_at, job_id, name, description, default_source_scheme, default_source_value, allow_override_source, default_destination_scheme, allow_override_destination, default_refresh, redisplay_value, validation_error, validation_condition, validation_type_system, validation_value_type, index FROM parameter_schemas WHERE created_at > $1 ` func (q *sqlQuerier) GetParameterSchemasCreatedAfter(ctx context.Context, createdAt time.Time) ([]ParameterSchema, error) { @@ -926,6 +929,7 @@ func (q *sqlQuerier) GetParameterSchemasCreatedAfter(ctx context.Context, create &i.ValidationCondition, &i.ValidationTypeSystem, &i.ValidationValueType, + &i.Index, ); err != nil { return nil, err } @@ -958,7 +962,8 @@ INSERT INTO validation_error, validation_condition, validation_type_system, - validation_value_type + validation_value_type, + index ) VALUES ( @@ -977,8 +982,9 @@ VALUES $13, $14, $15, - $16 - ) RETURNING id, created_at, job_id, name, description, default_source_scheme, default_source_value, allow_override_source, default_destination_scheme, allow_override_destination, default_refresh, redisplay_value, validation_error, validation_condition, validation_type_system, validation_value_type + $16, + $17 + ) RETURNING id, created_at, job_id, name, description, default_source_scheme, default_source_value, allow_override_source, default_destination_scheme, allow_override_destination, default_refresh, redisplay_value, validation_error, validation_condition, validation_type_system, validation_value_type, index ` type InsertParameterSchemaParams struct { @@ -998,6 +1004,7 @@ type InsertParameterSchemaParams struct { ValidationCondition string `db:"validation_condition" json:"validation_condition"` ValidationTypeSystem ParameterTypeSystem `db:"validation_type_system" json:"validation_type_system"` ValidationValueType string `db:"validation_value_type" json:"validation_value_type"` + Index int32 `db:"index" json:"index"` } func (q *sqlQuerier) InsertParameterSchema(ctx context.Context, arg InsertParameterSchemaParams) (ParameterSchema, error) { @@ -1018,6 +1025,7 @@ func (q *sqlQuerier) InsertParameterSchema(ctx context.Context, arg InsertParame arg.ValidationCondition, arg.ValidationTypeSystem, arg.ValidationValueType, + arg.Index, ) var i ParameterSchema err := row.Scan( @@ -1037,6 +1045,7 @@ func (q *sqlQuerier) InsertParameterSchema(ctx context.Context, arg InsertParame &i.ValidationCondition, &i.ValidationTypeSystem, &i.ValidationValueType, + &i.Index, ) return i, err } diff --git a/coderd/database/queries/parameterschemas.sql b/coderd/database/queries/parameterschemas.sql index 194a97930b6ca..77bef759a47a2 100644 --- a/coderd/database/queries/parameterschemas.sql +++ b/coderd/database/queries/parameterschemas.sql @@ -4,7 +4,9 @@ SELECT FROM parameter_schemas WHERE - job_id = $1; + job_id = $1 +ORDER BY + index; -- name: GetParameterSchemasCreatedAfter :many SELECT * FROM parameter_schemas WHERE created_at > $1; @@ -27,7 +29,8 @@ INSERT INTO validation_error, validation_condition, validation_type_system, - validation_value_type + validation_value_type, + index ) VALUES ( @@ -46,5 +49,6 @@ VALUES $13, $14, $15, - $16 + $16, + $17 ) RETURNING *; diff --git a/coderd/provisionerdaemons.go b/coderd/provisionerdaemons.go index 3fb23b4e5acd2..392f5270f9b50 100644 --- a/coderd/provisionerdaemons.go +++ b/coderd/provisionerdaemons.go @@ -405,7 +405,7 @@ func (server *provisionerdServer) UpdateJob(ctx context.Context, request *proto. } if len(request.ParameterSchemas) > 0 { - for _, protoParameter := range request.ParameterSchemas { + for index, protoParameter := range request.ParameterSchemas { validationTypeSystem, err := convertValidationTypeSystem(protoParameter.ValidationTypeSystem) if err != nil { return nil, xerrors.Errorf("convert validation type system for %q: %w", protoParameter.Name, err) @@ -428,6 +428,8 @@ func (server *provisionerdServer) UpdateJob(ctx context.Context, request *proto. AllowOverrideDestination: protoParameter.AllowOverrideDestination, AllowOverrideSource: protoParameter.AllowOverrideSource, + + Index: int32(index), } // It's possible a parameter doesn't define a default source! diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index a2cc37fc49931..498a1e349a492 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -244,17 +244,30 @@ func TestTemplateVersionParameters(t *testing.T) { Parse: []*proto.Parse_Response{{ Type: &proto.Parse_Response_Complete{ Complete: &proto.Parse_Complete{ - ParameterSchemas: []*proto.ParameterSchema{{ - Name: "example", - RedisplayValue: true, - DefaultSource: &proto.ParameterSource{ - Scheme: proto.ParameterSource_DATA, - Value: "hello", + ParameterSchemas: []*proto.ParameterSchema{ + { + Name: "example", + RedisplayValue: true, + DefaultSource: &proto.ParameterSource{ + Scheme: proto.ParameterSource_DATA, + Value: "hello", + }, + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, }, - DefaultDestination: &proto.ParameterDestination{ - Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + { + Name: "abcd", + RedisplayValue: true, + DefaultSource: &proto.ParameterSource{ + Scheme: proto.ParameterSource_DATA, + Value: "world", + }, + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, }, - }}, + }, }, }, }}, @@ -264,8 +277,9 @@ func TestTemplateVersionParameters(t *testing.T) { params, err := client.TemplateVersionParameters(context.Background(), version.ID) require.NoError(t, err) require.NotNil(t, params) - require.Len(t, params, 1) + require.Len(t, params, 2) require.Equal(t, "hello", params[0].SourceValue) + require.Equal(t, "world", params[1].SourceValue) }) } From a10c8c66ae49adc14ba2c5eb512c10791ca95e2b Mon Sep 17 00:00:00 2001 From: David Wahler Date: Wed, 13 Jul 2022 19:12:23 +0000 Subject: [PATCH 3/3] don't re-sort parameters by name when creating templates --- cli/templatecreate.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cli/templatecreate.go b/cli/templatecreate.go index 65ca968c9a67c..50d2e16619edd 100644 --- a/cli/templatecreate.go +++ b/cli/templatecreate.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "path/filepath" - "sort" "strings" "time" @@ -226,10 +225,6 @@ func createValidTemplateVersion(cmd *cobra.Command, args createValidTemplateVers valuesBySchemaID[parameterValue.SchemaID.String()] = parameterValue } - sort.Slice(parameterSchemas, func(i, j int) bool { - return parameterSchemas[i].Name < parameterSchemas[j].Name - }) - // parameterMapFromFile can be nil if parameter file is not specified var parameterMapFromFile map[string]string if args.ParameterFile != "" {