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

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 != "" {

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

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 (

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

@@ -0,0 +1 @@
+ALTER TABLE parameter_schemas DROP COLUMN index;

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;

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 {

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
+		$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
+		$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
 }

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 *;

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!

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{{
+						ParameterSchemas: []*proto.ParameterSchema{
-							Name:           "example",
+							{
-							RedisplayValue: true,
+								Name:           "example",
-							DefaultSource: &proto.ParameterSource{
+								RedisplayValue: true,
-								Scheme: proto.ParameterSource_DATA,
+								DefaultSource: &proto.ParameterSource{
-								Value:  "hello",
+									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)
 	})
 }
 

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
+}

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 {