From 0beafb377fb1fad7fd27a75d06387ccdf1465289 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Fri, 13 Jul 2018 15:24:35 -0700 Subject: [PATCH 1/5] Support unordered lists in Terraform. --- products/compute/terraform.yaml | 1 + provider/terraform/property_override.rb | 3 ++ templates/terraform/resource.erb | 11 ++++++ .../resource_definition/subnetwork.erb | 1 + .../unordered_list_customize_diff.erb | 34 +++++++++++++++++++ 5 files changed, 50 insertions(+) create mode 100644 templates/terraform/unordered_list_customize_diff.erb diff --git a/products/compute/terraform.yaml b/products/compute/terraform.yaml index e28ad70e9db0..05e3b03646ef 100644 --- a/products/compute/terraform.yaml +++ b/products/compute/terraform.yaml @@ -623,6 +623,7 @@ overrides: !ruby/object:Provider::ResourceOverrides function: 'validateGCPName' secondaryIpRanges: !ruby/object:Provider::Terraform::PropertyOverride name: secondaryIpRange + unordered_list: true secondaryIpRanges.rangeName: !ruby/object:Provider::Terraform::PropertyOverride validation: !ruby/object:Provider::Terraform::Validation function: 'validateGCPName' diff --git a/provider/terraform/property_override.rb b/provider/terraform/property_override.rb index 71dbdb9bbb84..f026bf643dc9 100644 --- a/provider/terraform/property_override.rb +++ b/provider/terraform/property_override.rb @@ -24,6 +24,8 @@ module OverrideFields attr_reader :state_func # Adds a StateFunc to the schema attr_reader :sensitive # Adds `Sensitive: true` to the schema attr_reader :validation # Adds a ValidateFunc to the schema + # Indicates that this is an Array that should have Set diff semantics. + attr_reader :unordered_list attr_reader :is_set # Uses a Set instead of an Array # Optional function to determine the unique ID of an item in the set @@ -92,6 +94,7 @@ def validate # Ensures boolean values are set to false if nil @sensitive ||= false @is_set ||= false + @unordered_list ||= false @default_from_api ||= false check_property :sensitive, :boolean diff --git a/templates/terraform/resource.erb b/templates/terraform/resource.erb index ef34c2bc1623..3c1920ca7901 100644 --- a/templates/terraform/resource.erb +++ b/templates/terraform/resource.erb @@ -36,6 +36,9 @@ func resource<%= resource_name -%>() *schema.Resource { Update: resource<%= resource_name -%>Update, <% end -%> Delete: resource<%= resource_name -%>Delete, + <% if settable_properties.any? {|p| p.unordered_list} && !object.custom_code.resource_definition -%> + CustomizeDiff: resource<%= resource_name -%>CustomizeDiff, + <% end -%> Importer: &schema.ResourceImporter{ State: resource<%= resource_name -%>Import, @@ -74,6 +77,14 @@ func resource<%= resource_name -%>() *schema.Resource { }, } } +<% if settable_properties.any? {|p| p.unordered_list} -%> +func resource<%= resource_name -%>CustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error { +<% settable_properties.select {|p| p.unordered_list}.each do |prop| -%> +<%= compile_template('templates/terraform/unordered_list_customize_diff.erb', + prop: prop) -%> +<% end -%> +} +<% end -%> func resource<%= resource_name -%>Create(d *schema.ResourceData, meta interface{}) error { config := meta.(*Config) diff --git a/templates/terraform/resource_definition/subnetwork.erb b/templates/terraform/resource_definition/subnetwork.erb index 4164a77de9c0..b1559ff9a8fb 100644 --- a/templates/terraform/resource_definition/subnetwork.erb +++ b/templates/terraform/resource_definition/subnetwork.erb @@ -1,3 +1,4 @@ CustomizeDiff: customdiff.All( customdiff.ForceNewIfChange("ip_cidr_range", isShrinkageIpCidr), + resourceComputeSubnetworkCustomizeDiff, ), diff --git a/templates/terraform/unordered_list_customize_diff.erb b/templates/terraform/unordered_list_customize_diff.erb new file mode 100644 index 000000000000..7bbd74819d88 --- /dev/null +++ b/templates/terraform/unordered_list_customize_diff.erb @@ -0,0 +1,34 @@ +keys := diff.GetChangedKeysPrefix(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>) +if len(keys) < 1 { + return nil +} +count := diff.Get("<%= Google::StringUtils.underscore(prop.name) -%>.#").(int) +if count < 1 { + return nil +} +old := make([]interface{}, count) +new := make([]interface{}, count) +for i := 0; i < count; i++ { + o, n := diff.GetChange(fmt.Sprintf("<%= Google::StringUtils.underscore(prop.name) -%>.%d", i)) + + if o != nil { + old = append(old, o) + } + if n != nil { + new = append(new, n) + } +} +if len(old) != len(new) { + return nil +} + +for o, _ := range old { + for n, _ := range new { + if reflect.DeepEqual(o, n) { + break + } + return nil + } +} + +return diff.Clear(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>) From 76056c48a8409064385c4b11ee14cd83a2650d40 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Mon, 16 Jul 2018 11:52:40 -0700 Subject: [PATCH 2/5] Use schema.Set instead of equality comparisons. --- products/compute/terraform.yaml | 1 + templates/terraform/resource.erb | 5 +++- .../unordered_list_customize_diff.erb | 28 +++++++++++++------ 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/products/compute/terraform.yaml b/products/compute/terraform.yaml index 05e3b03646ef..e39179fd9554 100644 --- a/products/compute/terraform.yaml +++ b/products/compute/terraform.yaml @@ -624,6 +624,7 @@ overrides: !ruby/object:Provider::ResourceOverrides secondaryIpRanges: !ruby/object:Provider::Terraform::PropertyOverride name: secondaryIpRange unordered_list: true + default_from_api: true secondaryIpRanges.rangeName: !ruby/object:Provider::Terraform::PropertyOverride validation: !ruby/object:Provider::Terraform::Validation function: 'validateGCPName' diff --git a/templates/terraform/resource.erb b/templates/terraform/resource.erb index 3c1920ca7901..ec0118ce4620 100644 --- a/templates/terraform/resource.erb +++ b/templates/terraform/resource.erb @@ -81,8 +81,11 @@ func resource<%= resource_name -%>() *schema.Resource { func resource<%= resource_name -%>CustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error { <% settable_properties.select {|p| p.unordered_list}.each do |prop| -%> <%= compile_template('templates/terraform/unordered_list_customize_diff.erb', - prop: prop) -%> + prop: prop, + resource_name: resource_name) -%> <% end -%> + +return nil } <% end -%> diff --git a/templates/terraform/unordered_list_customize_diff.erb b/templates/terraform/unordered_list_customize_diff.erb index 7bbd74819d88..c485ff1eb35d 100644 --- a/templates/terraform/unordered_list_customize_diff.erb +++ b/templates/terraform/unordered_list_customize_diff.erb @@ -2,7 +2,18 @@ keys := diff.GetChangedKeysPrefix(<%= go_literal(Google::StringUtils.underscore( if len(keys) < 1 { return nil } -count := diff.Get("<%= Google::StringUtils.underscore(prop.name) -%>.#").(int) + +oldCount, newCount := diff.GetChange("<%= Google::StringUtils.underscore(prop.name) -%>.#") +var count int +if oldCount.(int) < newCount.(int) { + count = newCount.(int) +} else { + count = oldCount.(int) +} +if count < 1 { + return nil +} + if count < 1 { return nil } @@ -18,17 +29,16 @@ for i := 0; i < count; i++ { new = append(new, n) } } + if len(old) != len(new) { return nil } -for o, _ := range old { - for n, _ := range new { - if reflect.DeepEqual(o, n) { - break - } - return nil +oldSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), old) +newSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), new) + +if oldSet.Equal(newSet) { + if err := diff.Clear(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>); err != nil { + return err } } - -return diff.Clear(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>) From 617ed278a7fab75ad33e4f3c3b8758175da99d0f Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Tue, 17 Jul 2018 11:43:51 -0700 Subject: [PATCH 3/5] Reorganize customize diff to be its own function for better functionality in event of multiple set-semantic lists. --- templates/terraform/resource.erb | 12 ++-- .../resource_definition/subnetwork.erb | 4 +- .../unordered_list_customize_diff.erb | 65 +++++++++---------- 3 files changed, 37 insertions(+), 44 deletions(-) diff --git a/templates/terraform/resource.erb b/templates/terraform/resource.erb index ec0118ce4620..1b3761539939 100644 --- a/templates/terraform/resource.erb +++ b/templates/terraform/resource.erb @@ -37,7 +37,11 @@ func resource<%= resource_name -%>() *schema.Resource { <% end -%> Delete: resource<%= resource_name -%>Delete, <% if settable_properties.any? {|p| p.unordered_list} && !object.custom_code.resource_definition -%> - CustomizeDiff: resource<%= resource_name -%>CustomizeDiff, + CustomizeDiff: customdiff.All( + <%= settable_properties.select { |p| p.unordered_list } + .map { |p| "resource#{resource_name}#{Google::StringUtils.camelize(p.name, :upper)}SetStyleDiff"} + .join(",\n")-%> + ), <% end -%> Importer: &schema.ResourceImporter{ @@ -77,15 +81,11 @@ func resource<%= resource_name -%>() *schema.Resource { }, } } -<% if settable_properties.any? {|p| p.unordered_list} -%> -func resource<%= resource_name -%>CustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error { <% settable_properties.select {|p| p.unordered_list}.each do |prop| -%> +func resource<%= resource_name -%><%= Google::StringUtils.camelize(prop.name, :upper) -%>SetStyleDiff(diff *schema.ResourceDiff, meta interface{}) error { <%= compile_template('templates/terraform/unordered_list_customize_diff.erb', prop: prop, resource_name: resource_name) -%> -<% end -%> - -return nil } <% end -%> diff --git a/templates/terraform/resource_definition/subnetwork.erb b/templates/terraform/resource_definition/subnetwork.erb index b1559ff9a8fb..b1d206878809 100644 --- a/templates/terraform/resource_definition/subnetwork.erb +++ b/templates/terraform/resource_definition/subnetwork.erb @@ -1,4 +1,4 @@ CustomizeDiff: customdiff.All( - customdiff.ForceNewIfChange("ip_cidr_range", isShrinkageIpCidr), - resourceComputeSubnetworkCustomizeDiff, + customdiff.ForceNewIfChange("ip_cidr_range", isShrinkageIpCidr), + resourceComputeSubnetworkSecondaryIpRangeSetStyleDiff, ), diff --git a/templates/terraform/unordered_list_customize_diff.erb b/templates/terraform/unordered_list_customize_diff.erb index c485ff1eb35d..8dbc90894eaa 100644 --- a/templates/terraform/unordered_list_customize_diff.erb +++ b/templates/terraform/unordered_list_customize_diff.erb @@ -1,44 +1,37 @@ keys := diff.GetChangedKeysPrefix(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>) -if len(keys) < 1 { - return nil -} - -oldCount, newCount := diff.GetChange("<%= Google::StringUtils.underscore(prop.name) -%>.#") -var count int -if oldCount.(int) < newCount.(int) { - count = newCount.(int) -} else { - count = oldCount.(int) -} -if count < 1 { - return nil -} - -if count < 1 { - return nil -} -old := make([]interface{}, count) -new := make([]interface{}, count) -for i := 0; i < count; i++ { - o, n := diff.GetChange(fmt.Sprintf("<%= Google::StringUtils.underscore(prop.name) -%>.%d", i)) - - if o != nil { - old = append(old, o) +if len(keys) > 0 { + oldCount, newCount := diff.GetChange("<%= Google::StringUtils.underscore(prop.name) -%>.#") + var count int + // There could be duplicates - worth continuing even if the counts are unequal. + if oldCount.(int) < newCount.(int) { + count = newCount.(int) + } else { + count = oldCount.(int) } - if n != nil { - new = append(new, n) + + if count < 1 { + return nil } -} + old := make([]interface{}, count) + new := make([]interface{}, count) + for i := 0; i < count; i++ { + o, n := diff.GetChange(fmt.Sprintf("<%= Google::StringUtils.underscore(prop.name) -%>.%d", i)) -if len(old) != len(new) { - return nil -} + if o != nil { + old = append(old, o) + } + if n != nil { + new = append(new, n) + } + } -oldSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), old) -newSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), new) + oldSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), old) + newSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), new) -if oldSet.Equal(newSet) { - if err := diff.Clear(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>); err != nil { - return err + if oldSet.Equal(newSet) { + if err := diff.Clear(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>); err != nil { + return err + } } } +return nil From 67b8bf1ec27b4ac2e3784ad85b3b53f9313a0ae0 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Tue, 17 Jul 2018 12:37:46 -0700 Subject: [PATCH 4/5] Travis maintainers recommend dropping this workaround. --- .travis.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index d8c9b188f996..1983139f2786 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,11 +1,5 @@ language: ruby # ruby version defined in .ruby-version will be used -before_install: -# some ruby versions come with a broken version of rubygems, update to -# consistent version -- gem update --system 2.7.6 -- gem install bundler -v '>= 1.16.1' - script: - bundle exec rake test:rubocop - bundle exec rake test:spec From 3b8431dcc5760e1bdf833534ea3cf449917c6824 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Tue, 17 Jul 2018 14:03:04 -0700 Subject: [PATCH 5/5] Reduce nesting. --- .../unordered_list_customize_diff.erb | 58 ++++++++++--------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/templates/terraform/unordered_list_customize_diff.erb b/templates/terraform/unordered_list_customize_diff.erb index 8dbc90894eaa..f308e10d9459 100644 --- a/templates/terraform/unordered_list_customize_diff.erb +++ b/templates/terraform/unordered_list_customize_diff.erb @@ -1,37 +1,39 @@ keys := diff.GetChangedKeysPrefix(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>) -if len(keys) > 0 { - oldCount, newCount := diff.GetChange("<%= Google::StringUtils.underscore(prop.name) -%>.#") - var count int - // There could be duplicates - worth continuing even if the counts are unequal. - if oldCount.(int) < newCount.(int) { - count = newCount.(int) - } else { - count = oldCount.(int) - } +if len(keys) == 0 { + return nil +} +oldCount, newCount := diff.GetChange("<%= Google::StringUtils.underscore(prop.name) -%>.#") +var count int +// There could be duplicates - worth continuing even if the counts are unequal. +if oldCount.(int) < newCount.(int) { + count = newCount.(int) +} else { + count = oldCount.(int) +} - if count < 1 { - return nil - } - old := make([]interface{}, count) - new := make([]interface{}, count) - for i := 0; i < count; i++ { - o, n := diff.GetChange(fmt.Sprintf("<%= Google::StringUtils.underscore(prop.name) -%>.%d", i)) +if count < 1 { + return nil +} +old := make([]interface{}, count) +new := make([]interface{}, count) +for i := 0; i < count; i++ { + o, n := diff.GetChange(fmt.Sprintf("<%= Google::StringUtils.underscore(prop.name) -%>.%d", i)) - if o != nil { - old = append(old, o) - } - if n != nil { - new = append(new, n) - } + if o != nil { + old = append(old, o) + } + if n != nil { + new = append(new, n) } +} - oldSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), old) - newSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), new) +oldSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), old) +newSet := schema.NewSet(schema.HashResource(resource<%= resource_name -%>().Schema[<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>].Elem.(*schema.Resource)), new) - if oldSet.Equal(newSet) { - if err := diff.Clear(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>); err != nil { - return err - } +if oldSet.Equal(newSet) { + if err := diff.Clear(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>); err != nil { + return err } } + return nil