From aa020c1f3d397cfa2da534759755ac2a668c4b4d Mon Sep 17 00:00:00 2001 From: Zheng Qin Date: Tue, 17 Sep 2024 20:40:32 -0400 Subject: [PATCH] feat: sort the variables in the same order from input variables.tf file. (#2591) Co-authored-by: Zheng Qin Co-authored-by: Andrew Peabody Co-authored-by: Bharath KKB --- .../int-test/goldens/golden-metadata.yaml | 256 +++++++++--------- cli/bpmetadata/tfconfig.go | 52 +++- cli/bpmetadata/tfconfig_test.go | 45 +++ .../bpmetadata/tf/empty-module/outputs.tf | 37 +++ .../bpmetadata/tf/empty-module/variables.tf | 34 +++ .../bpmetadata/tf/invalid-module/outputs.tf | 37 +++ .../bpmetadata/tf/invalid-module/variables.tf | 34 +++ 7 files changed, 364 insertions(+), 131 deletions(-) create mode 100644 cli/testdata/bpmetadata/tf/empty-module/outputs.tf create mode 100644 cli/testdata/bpmetadata/tf/empty-module/variables.tf create mode 100644 cli/testdata/bpmetadata/tf/invalid-module/outputs.tf create mode 100644 cli/testdata/bpmetadata/tf/invalid-module/variables.tf diff --git a/cli/bpmetadata/int-test/goldens/golden-metadata.yaml b/cli/bpmetadata/int-test/goldens/golden-metadata.yaml index 6059145ef0e..152aeed668d 100644 --- a/cli/bpmetadata/int-test/goldens/golden-metadata.yaml +++ b/cli/bpmetadata/int-test/goldens/golden-metadata.yaml @@ -26,10 +26,70 @@ spec: location: examples/simple_bucket interfaces: variables: + - name: project_id + description: Bucket project id. + varType: string + required: true + - name: prefix + description: Prefix used to generate the bucket name. + varType: string + defaultValue: "" + - name: names + description: Bucket name suffixes. + varType: list(string) + required: true + - name: randomize_suffix + description: Adds an identical, but randomized 4-character suffix to all bucket names + varType: bool + defaultValue: false + - name: location + description: Bucket location. + varType: string + defaultValue: EU + - name: storage_class + description: Bucket storage class. + varType: string + defaultValue: STANDARD + - name: force_destroy + description: Optional map of lowercase unprefixed name => boolean, defaults to false. + varType: map(bool) + defaultValue: {} + - name: versioning + description: Optional map of lowercase unprefixed name => boolean, defaults to false. + varType: map(bool) + defaultValue: {} + - name: encryption_key_names + description: Optional map of lowercase unprefixed name => string, empty strings are ignored. + varType: map(string) + defaultValue: {} + - name: bucket_policy_only + description: Disable ad-hoc ACLs on specified buckets. Defaults to true. Map of lowercase unprefixed name => boolean + varType: map(bool) + defaultValue: {} + - name: default_event_based_hold + description: Enable event based hold to new objects added to specific bucket. Defaults to false. Map of lowercase unprefixed name => boolean + varType: map(bool) + defaultValue: {} - name: admins description: IAM-style members who will be granted roles/storage.objectAdmin on all buckets. varType: list(string) defaultValue: [] + - name: creators + description: IAM-style members who will be granted roles/storage.objectCreators on all buckets. + varType: list(string) + defaultValue: [] + - name: viewers + description: IAM-style members who will be granted roles/storage.objectViewer on all buckets. + varType: list(string) + defaultValue: [] + - name: hmac_key_admins + description: IAM-style members who will be granted roles/storage.hmacKeyAdmin on all buckets. + varType: list(string) + defaultValue: [] + - name: storage_admins + description: IAM-style members who will be granted roles/storage.admin on all buckets. + varType: list(string) + defaultValue: [] - name: bucket_admins description: Map of lowercase unprefixed name => comma-delimited IAM-style per-bucket admins. varType: map(string) @@ -38,14 +98,50 @@ spec: description: Map of lowercase unprefixed name => comma-delimited IAM-style per-bucket creators. varType: map(string) defaultValue: {} + - name: bucket_viewers + description: Map of lowercase unprefixed name => comma-delimited IAM-style per-bucket viewers. + varType: map(string) + defaultValue: {} - name: bucket_hmac_key_admins description: Map of lowercase unprefixed name => comma-delimited IAM-style per-bucket HMAC Key admins. varType: map(string) defaultValue: {} - - name: bucket_lifecycle_rules - description: Additional lifecycle_rules for specific buckets. Map of lowercase unprefixed name => list of lifecycle rules to configure. + - name: bucket_storage_admins + description: Map of lowercase unprefixed name => comma-delimited IAM-style per-bucket storage admins. + varType: map(string) + defaultValue: {} + - name: labels + description: Labels to be attached to the buckets + varType: map(string) + defaultValue: {} + - name: folders + description: Map of lowercase unprefixed name => list of top level folder objects. + varType: map(list(string)) + defaultValue: {} + - name: set_admin_roles + description: Grant roles/storage.objectAdmin role to admins and bucket_admins. + varType: bool + defaultValue: false + - name: set_creator_roles + description: Grant roles/storage.objectCreator role to creators and bucket_creators. + varType: bool + defaultValue: false + - name: set_viewer_roles + description: Grant roles/storage.objectViewer role to viewers and bucket_viewers. + varType: bool + defaultValue: false + - name: set_hmac_key_admin_roles + description: Grant roles/storage.hmacKeyAdmin role to hmac_key_admins and bucket_hmac_key_admins. + varType: bool + defaultValue: false + - name: set_storage_admin_roles + description: Grant roles/storage.admin role to storage_admins and bucket_storage_admins. + varType: bool + defaultValue: false + - name: lifecycle_rules + description: List of lifecycle rules to configure. Format is the same as described in provider documentation https://www.terraform.io/docs/providers/google/r/storage_bucket.html#lifecycle_rule except condition.matches_storage_class should be a comma delimited string. varType: |- - map(set(object({ + set(object({ # Object with keys: # - type - The type of the action of this Lifecycle Rule. Supported values: Delete and SetStorageClass. # - storage_class - (Required if action type is SetStorageClass) The target Storage Class of objects affected by this Lifecycle Rule. @@ -56,70 +152,20 @@ spec: # - created_before - (Optional) Creation date of an object in RFC 3339 (e.g. 2017-06-13) to satisfy this condition. # - with_state - (Optional) Match to live and/or archived objects. Supported values include: "LIVE", "ARCHIVED", "ANY". # - matches_storage_class - (Optional) Comma delimited string for storage class of objects to satisfy this condition. Supported values include: MULTI_REGIONAL, REGIONAL, NEARLINE, COLDLINE, STANDARD, DURABLE_REDUCED_AVAILABILITY. + # - matches_prefix - (Optional) One or more matching name prefixes to satisfy this condition. + # - matches_suffix - (Optional) One or more matching name suffixes to satisfy this condition. # - num_newer_versions - (Optional) Relevant only for versioned objects. The number of newer versions of an object to satisfy this condition. # - custom_time_before - (Optional) A date in the RFC 3339 format YYYY-MM-DD. This condition is satisfied when the customTime metadata for the object is set to an earlier date than the date used in this lifecycle condition. # - days_since_custom_time - (Optional) The number of days from the Custom-Time metadata attribute after which this condition becomes true. # - days_since_noncurrent_time - (Optional) Relevant only for versioned objects. Number of days elapsed since the noncurrent timestamp of an object. # - noncurrent_time_before - (Optional) Relevant only for versioned objects. The date in RFC 3339 (e.g. 2017-06-13) when the object became nonconcurrent. condition = map(string) - }))) - defaultValue: {} - - name: bucket_policy_only - description: Disable ad-hoc ACLs on specified buckets. Defaults to true. Map of lowercase unprefixed name => boolean - varType: map(bool) - defaultValue: {} - - name: bucket_storage_admins - description: Map of lowercase unprefixed name => comma-delimited IAM-style per-bucket storage admins. - varType: map(string) - defaultValue: {} - - name: bucket_viewers - description: Map of lowercase unprefixed name => comma-delimited IAM-style per-bucket viewers. - varType: map(string) - defaultValue: {} - - name: cors - description: "Set of maps of mixed type attributes for CORS values. See appropriate attribute types here: https://www.terraform.io/docs/providers/google/r/storage_bucket.html#cors" - varType: set(any) - defaultValue: [] - - name: creators - description: IAM-style members who will be granted roles/storage.objectCreators on all buckets. - varType: list(string) - defaultValue: [] - - name: custom_placement_config - description: Map of lowercase unprefixed name => custom placement config object. Format is the same as described in provider documentation https://www.terraform.io/docs/providers/google/r/storage_bucket#custom_placement_config - varType: any - defaultValue: {} - - name: default_event_based_hold - description: Enable event based hold to new objects added to specific bucket. Defaults to false. Map of lowercase unprefixed name => boolean - varType: map(bool) - defaultValue: {} - - name: encryption_key_names - description: Optional map of lowercase unprefixed name => string, empty strings are ignored. - varType: map(string) - defaultValue: {} - - name: folders - description: Map of lowercase unprefixed name => list of top level folder objects. - varType: map(list(string)) - defaultValue: {} - - name: force_destroy - description: Optional map of lowercase unprefixed name => boolean, defaults to false. - varType: map(bool) - defaultValue: {} - - name: hmac_key_admins - description: IAM-style members who will be granted roles/storage.hmacKeyAdmin on all buckets. - varType: list(string) + })) defaultValue: [] - - name: hmac_service_accounts - description: List of HMAC service accounts to grant access to GCS. - varType: map(string) - defaultValue: {} - - name: labels - description: Labels to be attached to the buckets - varType: map(string) - defaultValue: {} - - name: lifecycle_rules - description: List of lifecycle rules to configure. Format is the same as described in provider documentation https://www.terraform.io/docs/providers/google/r/storage_bucket.html#lifecycle_rule except condition.matches_storage_class should be a comma delimited string. + - name: bucket_lifecycle_rules + description: Additional lifecycle_rules for specific buckets. Map of lowercase unprefixed name => list of lifecycle rules to configure. varType: |- - set(object({ + map(set(object({ # Object with keys: # - type - The type of the action of this Lifecycle Rule. Supported values: Delete and SetStorageClass. # - storage_class - (Required if action type is SetStorageClass) The target Storage Class of objects affected by this Lifecycle Rule. @@ -130,92 +176,46 @@ spec: # - created_before - (Optional) Creation date of an object in RFC 3339 (e.g. 2017-06-13) to satisfy this condition. # - with_state - (Optional) Match to live and/or archived objects. Supported values include: "LIVE", "ARCHIVED", "ANY". # - matches_storage_class - (Optional) Comma delimited string for storage class of objects to satisfy this condition. Supported values include: MULTI_REGIONAL, REGIONAL, NEARLINE, COLDLINE, STANDARD, DURABLE_REDUCED_AVAILABILITY. - # - matches_prefix - (Optional) One or more matching name prefixes to satisfy this condition. - # - matches_suffix - (Optional) One or more matching name suffixes to satisfy this condition. # - num_newer_versions - (Optional) Relevant only for versioned objects. The number of newer versions of an object to satisfy this condition. # - custom_time_before - (Optional) A date in the RFC 3339 format YYYY-MM-DD. This condition is satisfied when the customTime metadata for the object is set to an earlier date than the date used in this lifecycle condition. # - days_since_custom_time - (Optional) The number of days from the Custom-Time metadata attribute after which this condition becomes true. # - days_since_noncurrent_time - (Optional) Relevant only for versioned objects. Number of days elapsed since the noncurrent timestamp of an object. # - noncurrent_time_before - (Optional) Relevant only for versioned objects. The date in RFC 3339 (e.g. 2017-06-13) when the object became nonconcurrent. condition = map(string) - })) + }))) + defaultValue: {} + - name: cors + description: "Set of maps of mixed type attributes for CORS values. See appropriate attribute types here: https://www.terraform.io/docs/providers/google/r/storage_bucket.html#cors" + varType: set(any) defaultValue: [] - - name: location - description: Bucket location. - varType: string - defaultValue: EU - - name: logging - description: Map of lowercase unprefixed name => bucket logging config object. Format is the same as described in provider documentation https://www.terraform.io/docs/providers/google/r/storage_bucket.html#logging - varType: any + - name: website + description: "Map of website values. Supported attributes: main_page_suffix, not_found_page" + varType: map(any) defaultValue: {} - - name: names - description: Bucket name suffixes. - varType: list(string) - required: true - - name: prefix - description: Prefix used to generate the bucket name. - varType: string - defaultValue: "" - - name: project_id - description: Bucket project id. - varType: string - required: true - - name: public_access_prevention - description: Prevents public access to a bucket. Acceptable values are inherited or enforced. If inherited, the bucket uses public access prevention, only if the bucket is subject to the public access prevention organization policy constraint. - varType: string - defaultValue: inherited - - name: randomize_suffix - description: Adds an identical, but randomized 4-character suffix to all bucket names - varType: bool - defaultValue: false - name: retention_policy description: Map of retention policy values. Format is the same as described in provider documentation https://www.terraform.io/docs/providers/google/r/storage_bucket#retention_policy varType: any defaultValue: {} - - name: set_admin_roles - description: Grant roles/storage.objectAdmin role to admins and bucket_admins. - varType: bool - defaultValue: false - - name: set_creator_roles - description: Grant roles/storage.objectCreator role to creators and bucket_creators. - varType: bool - defaultValue: false + - name: custom_placement_config + description: Map of lowercase unprefixed name => custom placement config object. Format is the same as described in provider documentation https://www.terraform.io/docs/providers/google/r/storage_bucket#custom_placement_config + varType: any + defaultValue: {} + - name: logging + description: Map of lowercase unprefixed name => bucket logging config object. Format is the same as described in provider documentation https://www.terraform.io/docs/providers/google/r/storage_bucket.html#logging + varType: any + defaultValue: {} - name: set_hmac_access description: Set S3 compatible access to GCS. varType: bool defaultValue: false - - name: set_hmac_key_admin_roles - description: Grant roles/storage.hmacKeyAdmin role to hmac_key_admins and bucket_hmac_key_admins. - varType: bool - defaultValue: false - - name: set_storage_admin_roles - description: Grant roles/storage.admin role to storage_admins and bucket_storage_admins. - varType: bool - defaultValue: false - - name: set_viewer_roles - description: Grant roles/storage.objectViewer role to viewers and bucket_viewers. - varType: bool - defaultValue: false - - name: storage_admins - description: IAM-style members who will be granted roles/storage.admin on all buckets. - varType: list(string) - defaultValue: [] - - name: storage_class - description: Bucket storage class. - varType: string - defaultValue: STANDARD - - name: versioning - description: Optional map of lowercase unprefixed name => boolean, defaults to false. - varType: map(bool) - defaultValue: {} - - name: viewers - description: IAM-style members who will be granted roles/storage.objectViewer on all buckets. - varType: list(string) - defaultValue: [] - - name: website - description: "Map of website values. Supported attributes: main_page_suffix, not_found_page" - varType: map(any) + - name: hmac_service_accounts + description: List of HMAC service accounts to grant access to GCS. + varType: map(string) defaultValue: {} + - name: public_access_prevention + description: Prevents public access to a bucket. Acceptable values are inherited or enforced. If inherited, the bucket uses public access prevention, only if the bucket is subject to the public access prevention organization policy constraint. + varType: string + defaultValue: inherited outputs: - name: bucket description: Bucket resource (for single use). diff --git a/cli/bpmetadata/tfconfig.go b/cli/bpmetadata/tfconfig.go index d173f104aeb..bc5679ed444 100644 --- a/cli/bpmetadata/tfconfig.go +++ b/cli/bpmetadata/tfconfig.go @@ -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{ { @@ -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) } @@ -256,6 +273,35 @@ func getBlueprintInterfaces(configPath string) (*BlueprintInterface, error) { }, nil } +func getBlueprintVariableOrders(configPath string) (map[string]int, error) { + p := hclparse.NewParser() + variableFile, hclDiags := p.ParseHCLFile(filepath.Join(configPath, "variables.tf")) + err := hasHclErrors(hclDiags) + if hclDiags.HasErrors() { + return nil, err + } + variableContent, _, hclDiags := variableFile.Body.PartialContent(variableSchema) + 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{ diff --git a/cli/bpmetadata/tfconfig_test.go b/cli/bpmetadata/tfconfig_test.go index 05673ea375d..8752762c6c0 100644 --- a/cli/bpmetadata/tfconfig_test.go +++ b/cli/bpmetadata/tfconfig_test.go @@ -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 + }{ + { + 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 variable name should create nil order", + configPath: "empty-module", + expectOrders: map[string]int{}, + expectError: true, + }, + { + name: "No variable name should create nil order", + 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) + } + }) + } +} diff --git a/cli/testdata/bpmetadata/tf/empty-module/outputs.tf b/cli/testdata/bpmetadata/tf/empty-module/outputs.tf new file mode 100644 index 00000000000..49ae9051078 --- /dev/null +++ b/cli/testdata/bpmetadata/tf/empty-module/outputs.tf @@ -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, + ] +} diff --git a/cli/testdata/bpmetadata/tf/empty-module/variables.tf b/cli/testdata/bpmetadata/tf/empty-module/variables.tf new file mode 100644 index 00000000000..3d92e041dac --- /dev/null +++ b/cli/testdata/bpmetadata/tf/empty-module/variables.tf @@ -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 +} diff --git a/cli/testdata/bpmetadata/tf/invalid-module/outputs.tf b/cli/testdata/bpmetadata/tf/invalid-module/outputs.tf new file mode 100644 index 00000000000..49ae9051078 --- /dev/null +++ b/cli/testdata/bpmetadata/tf/invalid-module/outputs.tf @@ -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, + ] +} diff --git a/cli/testdata/bpmetadata/tf/invalid-module/variables.tf b/cli/testdata/bpmetadata/tf/invalid-module/variables.tf new file mode 100644 index 00000000000..f0fc8257b26 --- /dev/null +++ b/cli/testdata/bpmetadata/tf/invalid-module/variables.tf @@ -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 +}