[Terraform]: Start removing beta igm/rigm fields. #669

rileykarson · 2018-11-02T16:50:47Z

Hmm. I started on this resource thinking "I know [r]igm! This should be easy enough!" and I think I mostly got it right but rigm.update_strategy has bamboozled me - it doesn't do anything anymore but we forgot to deprecate it.

Not looking for feedback until Monday, just writing this down so I can remember after I context switch.

Part of hashicorp/terraform-provider-google#1203

Change named_ports to a Set
Remove rolling_update_policy, versions, auto_healing_policies from GA provider
Remove old self link stability test

[all]

[terraform]

Start removing beta igm/rigm fields.

[terraform-beta]

[ansible]

[inspec]

modular-magician · 2018-11-02T17:18:38Z

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.
depends: hashicorp/terraform-provider-google-beta#76
depends: hashicorp/terraform-provider-google#2393

modular-magician · 2018-11-02T18:50:21Z

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit b84475f) have been included in your existing downstream PRs.

modular-magician · 2018-11-07T23:55:09Z

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit e300035) have been included in your existing downstream PRs.

modular-magician · 2018-11-14T18:17:28Z

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit ad9533c) have been included in your existing downstream PRs.

paddycarver · 2018-11-19T20:14:18Z

third_party/terraform/resources/resource_compute_instance_group_manager.go.erb

@@ -215,7 +230,8 @@ func resourceComputeInstanceGroupManager() *schema.Resource {

 <% if version.nil? || version == 'ga' -%>
 			"rolling_update_policy": &schema.Schema{
-				Deprecated: "This field is in beta and will be removed from this provider. Use it in the the google-beta provider instead. See https://terraform.io/docs/providers/google/provider_versions.html for more details.",
+				Removed:    "This field is in beta. Use it in the the google-beta provider instead. See https://terraform.io/docs/providers/google/provider_versions.html for more details.",
+				Computed:   true,


Hmm, why do we need to set this to computed?

Afaik I got the impression at least once that you needed to do that in some edge case while removing some fields to avoid a diff, and have done so on any removal I've done since. Do you think we shouldn't have it set? Imo it's mostly harmless and as far as I can recall can only help.

paddycarver · 2018-11-19T20:16:19Z

third_party/terraform/resources/resource_compute_instance_group_manager.go.erb

@@ -525,14 +535,21 @@ func resourceComputeInstanceGroupManagerRead(d *schema.ResourceData, meta interf
 		update_strategy = "REPLACE"
 	}
 	d.Set("update_strategy", update_strategy.(string))
+
+	// Terraform _really_ doesn't like removing lists.


What does this mean? Why not? What happens?

Ah I ran into like 5 occurrences of this in the same day and was not very patient with it. I'll update the comment to be better.

We get a permadiffdiff on the .# of a list from 0 => (empty-as-in-nil).

Would a state migration fix this? I think if we just run a state migration to remove that field entirely, that's.... safer in the long run, I believe?

I don't think a state migration would work because this happens on new resources. The GKE CI tests failing the last few days were the same issue, fixed by #922

That's... concerning. If a field is marked as removed, it shouldn't wind up in state, I don't think? Let me verify that understanding really quick.

Just to verify: this permadiff happened when there were no d.Set calls setting the field, right? The d.Set was removed, the field was marked Removed, and then the tests were run, and they resulted in a permadiff?

Yes! This happens with TypeList and TypeSets that we mark Removed.

Very bizarre. Let me see if I can dig up some information.

paddycarver

Looks reasonable to me. I'm chasing down details on why Removed is awful, but let's not block on that, it's easy enough to delete code later.

modular-magician · 2018-11-19T22:56:25Z

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 804da96) have been included in your existing downstream PRs.

modular-magician · 2018-11-19T23:23:21Z

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit e5fd350) have been included in your existing downstream PRs.

rileykarson · 2018-11-19T23:30:00Z

Oh wait I am still not sure what we want to do with update_strategy in GA rigm.

It was never present at beta, and is only present at GA; it was previously used to turn on/off rolling updates, but they aren't present in GA anymore.

That means that the user can set it to NONE or... well, NONE is the only value now. It's a useless field.

The way I see it, we can:

Add the Terraform-driven update stuff from igm which we will then need to deprecate and remove in 3.0.0 once real updates go GA
Deprecate the field now and remove in 3.0.0. It won't do anything and any code interacting with it can be removed.
Remove the field now and woops we missed the deprecation on that one. But the field is useless so it isn't that bad.

I'd like to do #3 because then it's done now instead of later, but I'm happy to do #2 because we remove the Read code regardless.

paddycarver · 2018-11-19T23:39:40Z

I think my original plan with it was to leave it, so we could add in REPLACE later... and then I found out REPLACE didn't make sense (IIRC) and forgot to go back and deprecate it... I think?

I think I'd prefer option 2, because it doesn't hurt to just leave the field sitting around until 3.0.0 (I don't think), and we can deprecate it properly then. I know it's annoying to leave the schema there, but let's go ahead and deprecate it now, and we'll get it in the next round. Sorry for botching it this round.

rileykarson · 2018-11-20T17:42:36Z

OK, updated with #2 and then fixed tests.

The only notable bit is that update_policy was causing diffs in our released 1.19.0, not sure why CI didn't catch that before but it did yesterday. Marked it Computed to fix that.

modular-magician · 2018-11-20T17:56:15Z

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 26140ce) have been included in your existing downstream PRs.

paddycarver

I checked, and I think hardcoding to nil will be a noop in 0.12, not an error, which is what I was worried about. I'll still check when the 0.12 SDK is finalized, but I think we're fine to take that approach for now.

…Computed, and ImportStateVerifyIgnore-ing update_strategy for rigm.

rileykarson added the terraform label Nov 2, 2018

rileykarson requested a review from paddycarver November 2, 2018 16:50

googlebot added the cla: yes label Nov 2, 2018

modular-magician added the downstream-generated label Nov 2, 2018

rileykarson changed the title ~~[Terraform]: [WIP] Start removing beta igm/rigm fields.~~ [Terraform]: Start removing beta igm/rigm fields. Nov 7, 2018

rileykarson force-pushed the igm-rigm-removals branch from a621ea1 to b9bafab Compare November 14, 2018 17:57

paddycarver reviewed Nov 19, 2018

View reviewed changes

paddycarver approved these changes Nov 19, 2018

View reviewed changes

rileykarson requested a review from paddycarver November 19, 2018 23:30

rileykarson force-pushed the igm-rigm-removals branch from 930abd6 to 5ca3e1f Compare November 20, 2018 17:36

paddycarver approved these changes Nov 20, 2018

View reviewed changes

modular-magician added the automerged label Nov 21, 2018

rileykarson added 5 commits November 21, 2018 05:19

Start removing beta igm/rigm fields.

c75fa6f

Improve comment quality.

a842cda

Add the real error to the comment and not my bad memory of what it was.

5535e50

Deprecate update_strategy.

f4a17d7

Fix tests by stabilising rolling_update_policy, making update_policy …

5f6dc4c

…Computed, and ImportStateVerifyIgnore-ing update_strategy for rigm.

modular-magician force-pushed the igm-rigm-removals branch from 5ca3e1f to 5f6dc4c Compare November 21, 2018 05:20

modular-magician merged commit 9d19ec3 into GoogleCloudPlatform:master Nov 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Terraform]: Start removing beta igm/rigm fields. #669

[Terraform]: Start removing beta igm/rigm fields. #669

rileykarson commented Nov 2, 2018

modular-magician commented Nov 2, 2018

modular-magician commented Nov 2, 2018

modular-magician commented Nov 7, 2018

modular-magician commented Nov 14, 2018

paddycarver Nov 19, 2018

rileykarson Nov 19, 2018

paddycarver Nov 19, 2018

rileykarson Nov 19, 2018

paddycarver Nov 19, 2018

rileykarson Nov 19, 2018

paddycarver Nov 19, 2018

paddycarver Nov 19, 2018

rileykarson Nov 19, 2018 •

edited

Loading

paddycarver Nov 19, 2018

paddycarver left a comment

modular-magician commented Nov 19, 2018

modular-magician commented Nov 19, 2018

rileykarson commented Nov 19, 2018

paddycarver commented Nov 19, 2018

rileykarson commented Nov 20, 2018

modular-magician commented Nov 20, 2018

paddycarver left a comment

[Terraform]: Start removing beta igm/rigm fields. #669

[Terraform]: Start removing beta igm/rigm fields. #669

Conversation

rileykarson commented Nov 2, 2018

[all]

[terraform]

[terraform-beta]

[ansible]

[inspec]

modular-magician commented Nov 2, 2018

modular-magician commented Nov 2, 2018

modular-magician commented Nov 7, 2018

modular-magician commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileykarson Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver left a comment

Choose a reason for hiding this comment

modular-magician commented Nov 19, 2018

modular-magician commented Nov 19, 2018

rileykarson commented Nov 19, 2018

paddycarver commented Nov 19, 2018

rileykarson commented Nov 20, 2018

modular-magician commented Nov 20, 2018

paddycarver left a comment

Choose a reason for hiding this comment

rileykarson Nov 19, 2018 •

edited

Loading