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

Support unordered lists in Terraform. #351

Merged
merged 9 commits into from
Jul 17, 2018
6 changes: 0 additions & 6 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 2 additions & 0 deletions products/compute/terraform.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,8 @@ overrides: !ruby/object:Provider::ResourceOverrides
function: 'validateGCPName'
secondaryIpRanges: !ruby/object:Provider::Terraform::PropertyOverride
name: secondaryIpRange
unordered_list: true
default_from_api: true
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem comes from the fact that we're implicitly comparing against the returned value from the API - we wouldn't have any diff problems otherwise.

secondaryIpRanges.rangeName: !ruby/object:Provider::Terraform::PropertyOverride
validation: !ruby/object:Provider::Terraform::Validation
function: 'validateGCPName'
Expand Down
3 changes: 3 additions & 0 deletions provider/terraform/property_override.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions templates/terraform/resource.erb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ 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: 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{
State: resource<%= resource_name -%>Import,
Expand Down Expand Up @@ -74,6 +81,13 @@ func resource<%= resource_name -%>() *schema.Resource {
},
}
}
<% 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 -%>

func resource<%= resource_name -%>Create(d *schema.ResourceData, meta interface{}) error {
config := meta.(*Config)
Expand Down
3 changes: 2 additions & 1 deletion templates/terraform/resource_definition/subnetwork.erb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
CustomizeDiff: customdiff.All(
customdiff.ForceNewIfChange("ip_cidr_range", isShrinkageIpCidr),
customdiff.ForceNewIfChange("ip_cidr_range", isShrinkageIpCidr),
resourceComputeSubnetworkSecondaryIpRangeSetStyleDiff,
),
39 changes: 39 additions & 0 deletions templates/terraform/unordered_list_customize_diff.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
keys := diff.GetChangedKeysPrefix(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>)
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to return here if the counts aren't the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duplicates may exist. Added a comment.

count = newCount.(int)
} else {
count = oldCount.(int)
}

if count < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same as the lines immediately before it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

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

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 nil