Skip to content

Commit b5f5e90

Browse files
dwahlerdwahler
authored
Return template parameters in consistent order (#2975)
* return parameters from Terraform provisioner in sorted order * persist parameter indices in database and return them in correct order from API * don't re-sort parameters by name when creating templates
1 parent b692b7e commit b5f5e90

12 files changed

+141
-26
lines changed

‎cli/templatecreate.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"fmt"
55
"os"
66
"path/filepath"
7-
"sort"
87
"strings"
98
"time"
109

@@ -226,10 +225,6 @@ func createValidTemplateVersion(cmd *cobra.Command, args createValidTemplateVers
226225
valuesBySchemaID[parameterValue.SchemaID.String()] = parameterValue
227226
}
228227

229-
sort.Slice(parameterSchemas, func(i, j int) bool {
230-
return parameterSchemas[i].Name < parameterSchemas[j].Name
231-
})
232-
233228
// parameterMapFromFile can be nil if parameter file is not specified
234229
var parameterMapFromFile map[string]string
235230
if args.ParameterFile != "" {

‎coderd/database/databasefake/databasefake.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1027,6 +1027,9 @@ func (q *fakeQuerier) GetParameterSchemasByJobID(_ context.Context, jobID uuid.U
10271027
if len(parameters) == 0 {
10281028
return nil, sql.ErrNoRows
10291029
}
1030+
sort.Slice(parameters, func(i, j int) bool {
1031+
return parameters[i].Index < parameters[j].Index
1032+
})
10301033
return parameters, nil
10311034
}
10321035

@@ -1555,6 +1558,7 @@ func (q *fakeQuerier) InsertParameterSchema(_ context.Context, arg database.Inse
15551558
ValidationCondition: arg.ValidationCondition,
15561559
ValidationTypeSystem: arg.ValidationTypeSystem,
15571560
ValidationValueType: arg.ValidationValueType,
1561+
Index: arg.Index,
15581562
}
15591563
q.parameterSchemas = append(q.parameterSchemas, param)
15601564
return param, nil

‎coderd/database/dump.sql

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000029_variable_order.down.sql

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE parameter_schemas DROP COLUMN index;

‎coderd/database/migrations/000029_variable_order.up.sql

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
-- temporarily create index column as nullable
2+
ALTER TABLE parameter_schemas ADD COLUMN index integer;
3+
4+
-- initialize ordering for existing parameters
5+
WITH tmp AS (
6+
SELECT id, row_number() OVER (PARTITION BY job_id ORDER BY name) AS index
7+
FROM parameter_schemas
8+
)
9+
UPDATE parameter_schemas
10+
SET index = tmp.index
11+
FROM tmp
12+
WHERE parameter_schemas.id = tmp.id;
13+
14+
-- all rows should now be initialized
15+
ALTER TABLE parameter_schemas ALTER COLUMN index SET NOT NULL;

‎coderd/database/models.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 14 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/parameterschemas.sql

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ SELECT
44
FROM
55
parameter_schemas
66
WHERE
7-
job_id = $1;
7+
job_id = $1
8+
ORDER BY
9+
index;
810

911
-- name: GetParameterSchemasCreatedAfter :many
1012
SELECT * FROM parameter_schemas WHERE created_at > $1;
@@ -27,7 +29,8 @@ INSERT INTO
2729
validation_error,
2830
validation_condition,
2931
validation_type_system,
30-
validation_value_type
32+
validation_value_type,
33+
index
3134
)
3235
VALUES
3336
(
@@ -46,5 +49,6 @@ VALUES
4649
$13,
4750
$14,
4851
$15,
49-
$16
52+
$16,
53+
$17
5054
) RETURNING *;

‎coderd/provisionerdaemons.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ func (server *provisionerdServer) UpdateJob(ctx context.Context, request *proto.
405405
}
406406

407407
if len(request.ParameterSchemas) > 0 {
408-
for _, protoParameter := range request.ParameterSchemas {
408+
for index, protoParameter := range request.ParameterSchemas {
409409
validationTypeSystem, err := convertValidationTypeSystem(protoParameter.ValidationTypeSystem)
410410
if err != nil {
411411
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.
428428

429429
AllowOverrideDestination: protoParameter.AllowOverrideDestination,
430430
AllowOverrideSource: protoParameter.AllowOverrideSource,
431+
432+
Index: int32(index),
431433
}
432434

433435
// It's possible a parameter doesn't define a default source!

‎coderd/templateversions_test.go

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -244,17 +244,30 @@ func TestTemplateVersionParameters(t *testing.T) {
244244
Parse: []*proto.Parse_Response{{
245245
Type: &proto.Parse_Response_Complete{
246246
Complete: &proto.Parse_Complete{
247-
ParameterSchemas: []*proto.ParameterSchema{{
248-
Name: "example",
249-
RedisplayValue: true,
250-
DefaultSource: &proto.ParameterSource{
251-
Scheme: proto.ParameterSource_DATA,
252-
Value: "hello",
247+
ParameterSchemas: []*proto.ParameterSchema{
248+
{
249+
Name: "example",
250+
RedisplayValue: true,
251+
DefaultSource: &proto.ParameterSource{
252+
Scheme: proto.ParameterSource_DATA,
253+
Value: "hello",
254+
},
255+
DefaultDestination: &proto.ParameterDestination{
256+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
257+
},
253258
},
254-
DefaultDestination: &proto.ParameterDestination{
255-
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
259+
{
260+
Name: "abcd",
261+
RedisplayValue: true,
262+
DefaultSource: &proto.ParameterSource{
263+
Scheme: proto.ParameterSource_DATA,
264+
Value: "world",
265+
},
266+
DefaultDestination: &proto.ParameterDestination{
267+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
268+
},
256269
},
257-
}},
270+
},
258271
},
259272
},
260273
}},
@@ -264,8 +277,9 @@ func TestTemplateVersionParameters(t *testing.T) {
264277
params, err := client.TemplateVersionParameters(context.Background(), version.ID)
265278
require.NoError(t, err)
266279
require.NotNil(t, params)
267-
require.Len(t, params, 1)
280+
require.Len(t, params, 2)
268281
require.Equal(t, "hello", params[0].SourceValue)
282+
require.Equal(t, "world", params[1].SourceValue)
269283
})
270284
}
271285

‎provisioner/terraform/parse.go

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"os"
77
"path/filepath"
8+
"sort"
89
"strings"
910

1011
"github.com/hashicorp/terraform-config-inspect/tfconfig"
@@ -22,8 +23,17 @@ func (*server) Parse(request *proto.Parse_Request, stream proto.DRPCProvisioner_
2223
return xerrors.Errorf("load module: %s", formatDiagnostics(request.Directory, diags))
2324
}
2425

25-
parameters := make([]*proto.ParameterSchema, 0, len(module.Variables))
26+
// Sort variables by (filename, line) to make the ordering consistent
27+
variables := make([]*tfconfig.Variable, 0, len(module.Variables))
2628
for _, v := range module.Variables {
29+
variables = append(variables, v)
30+
}
31+
sort.Slice(variables, func(i, j int) bool {
32+
return compareSourcePos(variables[i].Pos, variables[j].Pos)
33+
})
34+
35+
parameters := make([]*proto.ParameterSchema, 0, len(variables))
36+
for _, v := range variables {
2737
schema, err := convertVariableToParameter(v)
2838
if err != nil {
2939
return xerrors.Errorf("convert variable %q: %w", v.Name, err)
@@ -129,3 +139,10 @@ func formatDiagnostics(baseDir string, diags tfconfig.Diagnostics) string {
129139

130140
return spacer + strings.TrimSpace(msgs.String())
131141
}
142+
143+
func compareSourcePos(x, y tfconfig.SourcePos) bool {
144+
if x.Filename != y.Filename {
145+
return x.Filename < y.Filename
146+
}
147+
return x.Line < y.Line
148+
}

‎provisioner/terraform/parse_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,58 @@ func TestParse(t *testing.T) {
128128
},
129129
ErrorContains: `The ";" character is not valid.`,
130130
},
131+
{
132+
Name: "multiple-variables",
133+
Files: map[string]string{
134+
"main1.tf": `variable "foo" { }
135+
variable "bar" { }`,
136+
"main2.tf": `variable "baz" { }
137+
variable "quux" { }`,
138+
},
139+
Response: &proto.Parse_Response{
140+
Type: &proto.Parse_Response_Complete{
141+
Complete: &proto.Parse_Complete{
142+
ParameterSchemas: []*proto.ParameterSchema{
143+
{
144+
Name: "foo",
145+
RedisplayValue: true,
146+
AllowOverrideSource: true,
147+
Description: "",
148+
DefaultDestination: &proto.ParameterDestination{
149+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
150+
},
151+
},
152+
{
153+
Name: "bar",
154+
RedisplayValue: true,
155+
AllowOverrideSource: true,
156+
Description: "",
157+
DefaultDestination: &proto.ParameterDestination{
158+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
159+
},
160+
},
161+
{
162+
Name: "baz",
163+
RedisplayValue: true,
164+
AllowOverrideSource: true,
165+
Description: "",
166+
DefaultDestination: &proto.ParameterDestination{
167+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
168+
},
169+
},
170+
{
171+
Name: "quux",
172+
RedisplayValue: true,
173+
AllowOverrideSource: true,
174+
Description: "",
175+
DefaultDestination: &proto.ParameterDestination{
176+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
177+
},
178+
}},
179+
},
180+
},
181+
},
182+
},
131183
}
132184

133185
for _, testCase := range testCases {

0 commit comments

Comments
 (0)