Support unordered lists in Terraform. #351
Conversation
|
I am a robot that works on MagicModules PRs! I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
| return nil | ||
| } | ||
|
|
||
| for o, _ := range old { |
There was a problem hiding this comment.
This logic seems wrong to me. First off, o and n are the array indices. Second, this looks like it'll return nil (i.e. keep doing what you're doing) as soon as any two elements aren't equal. What if instead, we loaded everything into a schema.Set and then checked equality on that?
There was a problem hiding this comment.
Weird that the tests passed - yikes, thanks for catching that.
Loading into a schema.Set is a good plan, I'll try that. I tried with regular go sets, but of course that didn't work since []interface{} isn't hashable.
There was a problem hiding this comment.
That's much better, thank you.
|
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 5226e73) have been included in your existing downstream PRs. |
| @@ -1,3 +1,4 @@ | |||
| CustomizeDiff: customdiff.All( | |||
| customdiff.ForceNewIfChange("ip_cidr_range", isShrinkageIpCidr), | |||
| resourceComputeSubnetworkCustomizeDiff, | |||
There was a problem hiding this comment.
nit: mind making this line tabs or the previous line spaces?
| @@ -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 | |||
There was a problem hiding this comment.
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.
|
|
||
| oldCount, newCount := diff.GetChange("<%= Google::StringUtils.underscore(prop.name) -%>.#") | ||
| var count int | ||
| if oldCount.(int) < newCount.(int) { |
There was a problem hiding this comment.
Does it make sense to return here if the counts aren't the same?
There was a problem hiding this comment.
Duplicates may exist. Added a comment.
| return nil | ||
| } | ||
|
|
||
| if count < 1 { |
There was a problem hiding this comment.
This is the same as the lines immediately before it
…lity in event of multiple set-semantic lists.
|
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 368f480) have been included in your existing downstream PRs. |
| @@ -0,0 +1,37 @@ | |||
| keys := diff.GetChangedKeysPrefix(<%= go_literal(Google::StringUtils.underscore(prop.name)) -%>) | |||
| if len(keys) > 0 { | |||
There was a problem hiding this comment.
nit: I would personally find it easier to follow if it were
if len(keys) == 0 {
return nil
}
but up to you
There was a problem hiding this comment.
Agreed, fixed and working its way through.
| 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. |
There was a problem hiding this comment.
Shouldn't this be a diff though?
Old New
-a -a
-a -b
-b
There was a problem hiding this comment.
Things marked as unordered lists are all intended to be changed to sets in 2.0.0 - it's better to not have user-visible changes when that happens. So I think this should not be a diff! :)
|
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit a85cfc0) have been included in your existing downstream PRs. |
Tracked submodules are build/puppet/compute build/puppet/sql build/puppet/storage build/puppet/container build/puppet/dns build/puppet/pubsub build/puppet/resourcemanager build/chef/compute build/chef/sql build/chef/storage build/chef/container build/chef/dns build/terraform build/ansible.
Some things are not ordered on the server side. This supports a customizediff for lists that are not ordered, and applies that to subnetwork's secondary IP ranges.
[all]
[terraform]
Support unordered list on subnetwork secondary IP range.
[puppet]
[puppet-compute]
[puppet-container]
[puppet-dns]
[puppet-logging]
[puppet-pubsub]
[puppet-resourcemanager]
[puppet-sql]
[puppet-storage]
[chef]
[chef-compute]
[chef-container]
[chef-dns]
[chef-logging]
[chef-sql]
[chef-storage]
[ansible]