Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: sort the variables in the same order from input variables.tf file. #2591

Merged
merged 12 commits into from
Sep 18, 2024
256 changes: 128 additions & 128 deletions cli/bpmetadata/int-test/goldens/golden-metadata.yaml

Large diffs are not rendered by default.

52 changes: 49 additions & 3 deletions cli/bpmetadata/tfconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ var metaSchema = &hcl.BodySchema{
},
}

var variableSchema = &hcl.BodySchema{
Blocks: []hcl.BlockHeaderSchema{
{
Type: "variable",
LabelNames: []string{"name"},
},
},
}

var metaBlockSchema = &hcl.BodySchema{
Attributes: []hcl.AttributeSchema{
{
Expand Down Expand Up @@ -237,13 +246,21 @@ func getBlueprintInterfaces(configPath string) (*BlueprintInterface, error) {
variables = append(variables, v)
}

// Sort variables
sort.SliceStable(variables, func(i, j int) bool { return variables[i].Name < variables[j].Name })
// Get the varible orders from tf file.
variableOrders, sortErr := getBlueprintVariableOrders(configPath)
if sortErr != nil {
Log.Info("Failed to get variables orders. Fallback to sort by variable names.", sortErr)
sort.SliceStable(variables, func(i, j int) bool { return variables[i].Name < variables[j].Name })
} else {
Log.Info("Sort variables by the original input order.")
sort.SliceStable(variables, func(i, j int) bool {
return variableOrders[variables[i].Name] < variableOrders[variables[j].Name]
})
}

var outputs []*BlueprintOutput
for _, val := range mod.Outputs {
o := getBlueprintOutput(val)

outputs = append(outputs, o)
}

Expand All @@ -256,6 +273,35 @@ func getBlueprintInterfaces(configPath string) (*BlueprintInterface, error) {
}, nil
}

func getBlueprintVariableOrders(configPath string) (map[string]int, error) {
p := hclparse.NewParser()
qz267 marked this conversation as resolved.
Show resolved Hide resolved
variableFile, hclDiags := p.ParseHCLFile(filepath.Join(configPath, "variables.tf"))
err := hasHclErrors(hclDiags)
if hclDiags.HasErrors() {
return nil, err
}
variableContent, _, hclDiags := variableFile.Body.PartialContent(variableSchema)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be problem here if there was an error in variables.tf file parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about calling log.Error() instead? Or do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My recommendation would be to propagate the error from this function (change signature to (map[string]int, error) and only sort if no error, otherwise log and continue so we degrade gracefully. IRL if this throws an error, we would not reach here as it will likely be caught earlier in tfconfig.LoadModule.

err = hasHclErrors(hclDiags)
if hclDiags.HasErrors() {
return nil, err
}
variableOrderKeys := make(map[string]int)
for i, block := range variableContent.Blocks {
// We only care about variable blocks.
if block.Type != "variable" {
continue
}
// We expect a single label which is the variable name.
if len(block.Labels) != 1 || len(block.Labels[0]) == 0 {
Log.Info("zheng: called here.")
return nil, fmt.Errorf("Vaiable block has no name.")
}

variableOrderKeys[block.Labels[0]] = i
}
return variableOrderKeys, nil
}

// build variable
func getBlueprintVariable(modVar *tfconfig.Variable) *BlueprintVariable {
v := &BlueprintVariable{
Expand Down
45 changes: 45 additions & 0 deletions cli/bpmetadata/tfconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,3 +368,48 @@ func TestTFIncompleteProviderVersions(t *testing.T) {
})
}
}

func TestTFVariableSortOrder(t *testing.T) {
tests := []struct {
name string
configPath string
expectOrders map[string]int
expectError bool
}{
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a few more cases with no outputs and invalid output block declaration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

name: "Variable order should match tf input",
configPath: "sample-module",
expectOrders: map[string]int{
"description": 1,
"project_id": 0,
"regional": 2,
},
expectError: false,
},
{
name: "Empty vairable name should create nil order",
bharathkkb marked this conversation as resolved.
Show resolved Hide resolved
configPath: "empty-module",
expectOrders: map[string]int{},
expectError: true,
},
{
name: "No vairable name should create nil order",
bharathkkb marked this conversation as resolved.
Show resolved Hide resolved
configPath: "invalid-module",
expectOrders: map[string]int{},
expectError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := getBlueprintVariableOrders(path.Join(tfTestdataPath, tt.configPath))
if tt.expectError {
assert.Error(t, err)
assert.Nil(t, got)
} else {
require.NoError(t, err)
assert.Equal(t, got, tt.expectOrders)
}
})
}
}
37 changes: 37 additions & 0 deletions cli/testdata/bpmetadata/tf/empty-module/outputs.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* Copyright 2022 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// This file was automatically generated from a template in ./autogen/main

output "cluster_id" {
description = "Cluster ID"
}

output "endpoint" {
sensitive = true
description = "Cluster endpoint"
value = local.cluster_endpoint
depends_on = [
/* Nominally, the endpoint is populated as soon as it is known to Terraform.
* However, the cluster may not be in a usable state yet. Therefore any
* resources dependent on the cluster being up will fail to deploy. With
* this explicit dependency, dependent resources can wait for the cluster
* to be up.
*/
google_container_cluster.primary,
google_container_node_pool.pools,
]
}
34 changes: 34 additions & 0 deletions cli/testdata/bpmetadata/tf/empty-module/variables.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Copyright 2022 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// This file was automatically generated from a template in ./autogen/main

variable "" { // Empty variable name
description = "The project ID to host the cluster in"
required = false
}

variable "description" {
description = "The description of the cluster"
type = string
default = "some description"
}

variable "regional" {
type = bool
description = "Whether is a regional cluster"
default = true
}
37 changes: 37 additions & 0 deletions cli/testdata/bpmetadata/tf/invalid-module/outputs.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* Copyright 2022 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// This file was automatically generated from a template in ./autogen/main

output "cluster_id" {
description = "Cluster ID"
}

output "endpoint" {
sensitive = true
description = "Cluster endpoint"
value = local.cluster_endpoint
depends_on = [
/* Nominally, the endpoint is populated as soon as it is known to Terraform.
* However, the cluster may not be in a usable state yet. Therefore any
* resources dependent on the cluster being up will fail to deploy. With
* this explicit dependency, dependent resources can wait for the cluster
* to be up.
*/
google_container_cluster.primary,
google_container_node_pool.pools,
]
}
34 changes: 34 additions & 0 deletions cli/testdata/bpmetadata/tf/invalid-module/variables.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Copyright 2022 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// This file was automatically generated from a template in ./autogen/main

variable { // No variable name
description = "The project ID to host the cluster in"
required = false
}

variable "description" {
description = "The description of the cluster"
type = string
default = "some description"
}

variable "regional" {
type = bool
description = "Whether is a regional cluster"
default = true
}