Fix perma-diff on instance templates. #1995

paddycarver · 2018-09-06T00:42:29Z

When using instance templates, if you use one of our image shorthands at
the moment, you'll get a perma-diff. This is because the config gets
resolved to another format before it is set in state, and so we need to
set that other format in state.

Unfortunately, resolving images requires network access, so we have to
do this with CustomizeDiff. CustomizeDiff was having trouble (I think?
More on this below) on setting a field as not ForceNew once the field
was already set, so I moved the decision for whether a field was
ForceNew or not into CustomizeDiff. I also resolved the old and new
images, and if they were the same, cleared the diff.

Unfortunately, you can't actually clear a field on a sub-block right
now. You have to clear top-level fields only. So this will currently
throw an error. I opened hashicorp/terraform#18795 to fix that. Once
that's merged, and we vendor it here, this patch fixes the problem.

If hashicorp/terraform#18795 doesn't get merged, the next best
workaround is to keep track of all the fields under disk with a diff
in our CustomizeDiff, check whether they've all changed or not, and if
they've all changed, clear the changes on disk, which I think will
resolve the issue. That's just a massive pain, unfortunately.

When using instance templates, if you use one of our image shorthands at the moment, you'll get a perma-diff. This is because the config gets resolved to another format before it is set in state, and so we need to set that other format in state. Unfortunately, resolving images requires network access, so we have to do this with CustomizeDiff. CustomizeDiff was having trouble (I think? More on this below) on setting a field as not ForceNew once the field was already set, so I moved the decision for whether a field was ForceNew or not into CustomizeDiff. I also resolved the old and new images, and if they were the same, cleared the diff. Unfortunately, you can't actually clear a field on a sub-block right now. You have to clear top-level fields only. So this will currently throw an error. I opened hashicorp/terraform#18795 to fix that. Once that's merged, and we vendor it here, this patch fixes the problem. If hashicorp/terraform#18795 doesn't get merged, the next best workaround is to keep track of _all_ the fields under `disk` with a diff in our CustomizeDiff, check whether they've all changed or not, and if they've all changed, clear the changes on `disk`, which I _think_ will resolve the issue. That's just a massive pain, unfortunately.

danawillow · 2018-09-20T23:40:03Z

Does this fix #2067?

paddycarver · 2018-09-20T23:45:26Z

I don't believe so. This will fix a permadiff, #2067 requires a bit of logic/parsing in the flattenDisks helper for instance_template. I'll update that tomorrow.

Update the version of helper/schema we have vendored to take advantage of fixes to the ResourceDiff.Clear function.

danawillow · 2018-10-01T16:47:36Z

vendor/vendor.json

-			"revision": "41e50bd32a8825a84535e353c3674af8ce799161",
-			"revisionTime": "2018-04-10T16:50:42Z",
+			"revision": "35d82b055591e9d47a254e68754216d8849ba67a",
+			"revisionTime": "2018-09-26T21:21:28Z",


It's probably not actually a big deal at all, but this has a versionExact and a revision time that is definitely a few versions ahead of the versionExact. Any idea how that happened?

If I had to guess, it's a result of govendor updating the revision and revisionTime, but not the version or versionExact. I've gone ahead and removed the version and versionExact.

danawillow · 2018-10-01T16:50:29Z

google/resource_compute_instance_template.go

@@ -420,6 +420,48 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 	}
 }

+func resourceComputeInstanceTemplateCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error {


nit: I'd call this resourceComputeInstanceTemplateSourceImageCustomizeDiff, since presumably if we added more fields to customize we would want to do a customdiff.All to group them.

(also lol, every so often i think that maybe our resources should be like, objects, so we don't have to have these ridiculously long globally-unique function names)

Fair! I've been doing massive monolithic CustomizeDiff functions, but I think customdiff.All--which I just learned about!--is a better approach.

And yeah, long names are amazing. -_-

danawillow · 2018-10-01T16:53:56Z

google/resource_compute_instance_template.go

+				err = diff.ForceNew(key)
+				if err != nil {
+					return err
+				}


shouldn't there be a continue here? otherwise the diff will get cleared on line 456

Good catch.

Fix Dana's comments, one of which exposed a bug: we weren't actually fixing the diff. Because resolveImage can return multiple formats, and only returns a self_link if a self_link is passed in, it was diffing more often than not against a self_link supplied in the config. We just had a bug in our CustomizeDiff function that concealed this. I added the resolvedImageSelfLink helper function to turn the output from resolveImage into a self_link, which fixes the problem. It also has the knock-on effect of fixing #2067 at the same time, which is nice. I decided to add another function instead of just modifying resolveImage to always return a self_link because I don't want to deal with the backwards compatibility problems of changing how we're storing a bunch of things in state this close to 1.19.0. And honestly, we should probably just be storing the self_link always, _anyways_.

paddycarver · 2018-10-02T08:16:52Z

I mentioned this in the commit, but the missing continue that @danawillow pointed out was actually masking that this didn't really fix the problem. Because resolveImage can return multiple formats, and only returns a self_link if a self_link is passed in, it was diffing more often than not against a self_link supplied in the config. The missing continue just concealed this.

I added the resolvedImageSelfLink helper function to turn the output from resolveImage into a self_link, which fixes the problem. It also has the knock-on effect of fixing #2067 at the same time, which is nice. I decided to add another function instead of just modifying resolveImage
to always return a self_link because I don't want to deal with the backwards compatibility problems of changing how we're storing a bunch of things in state this close to 1.19.0. And honestly, we should
probably just be storing the self_link always, anyways.

JSON is the worst.

danawillow

How does this fix #2067? That calls getRelativePath on a field that got returned from the API, and this doesn't change that.

Anyway, I'm not thrilled with this solution, but assuming that it does indeed work, I'll say it's fine. Can you file an issue about this to make sure it's something that we can make better in 2.0.0?

paddycarver · 2018-10-02T20:41:05Z

Oh, you're right. We'd still need to use resolvedImageSelfLink here: https://github.com/terraform-providers/terraform-provider-google/blob/27bd76d9d40d0149d75a1489f83eba119a3b01d5/google/resource_compute_instance_template.go#L640

But this at least sets up a small PR to fix that?

paddycarver · 2018-10-02T21:13:11Z

Can you file an issue about this to make sure it's something that we can make better in 2.0.0?

#2151

Going to go ahead and merge this now.

ghost · 2018-11-16T13:56:31Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

ghost added the size/m label Sep 6, 2018

paddycarver mentioned this pull request Sep 6, 2018

Always set instance template source images on read. #1916

Merged

danisla mentioned this pull request Sep 21, 2018

compute_image forces re-creation of compute_instance_template on every terraform plan/apply GoogleCloudPlatform/terraform-google-managed-instance-group#28

Closed

Update helper/schema vendor.

75c20b4

Update the version of helper/schema we have vendored to take advantage of fixes to the ResourceDiff.Clear function.

paddycarver requested a review from danawillow September 26, 2018 21:25

paddycarver added this to the 1.19.0 milestone Sep 30, 2018

danawillow reviewed Oct 1, 2018

View reviewed changes

ghost added size/l and removed size/m labels Oct 2, 2018

Fix trailing comma

a8fdf40

JSON is the worst.

danawillow approved these changes Oct 2, 2018

View reviewed changes

paddycarver mentioned this pull request Oct 2, 2018

Always set self_link in state for instance_templates #2151

Closed

paddycarver merged commit c1d404d into master Oct 2, 2018

paultyng deleted the paddy_1916_diff_fix branch October 29, 2018 19:27

ghost locked and limited conversation to collaborators Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix perma-diff on instance templates. #1995

Fix perma-diff on instance templates. #1995

paddycarver commented Sep 6, 2018

danawillow commented Sep 20, 2018

paddycarver commented Sep 20, 2018

danawillow Oct 1, 2018

paddycarver Oct 2, 2018

danawillow Oct 1, 2018

paddycarver Oct 2, 2018

danawillow Oct 1, 2018

paddycarver Oct 2, 2018

paddycarver commented Oct 2, 2018

danawillow left a comment

paddycarver commented Oct 2, 2018

paddycarver commented Oct 2, 2018

ghost commented Nov 16, 2018

Fix perma-diff on instance templates. #1995

Fix perma-diff on instance templates. #1995

Conversation

paddycarver commented Sep 6, 2018

danawillow commented Sep 20, 2018

paddycarver commented Sep 20, 2018

danawillow Oct 1, 2018

Choose a reason for hiding this comment

paddycarver Oct 2, 2018

Choose a reason for hiding this comment

danawillow Oct 1, 2018

Choose a reason for hiding this comment

paddycarver Oct 2, 2018

Choose a reason for hiding this comment

danawillow Oct 1, 2018

Choose a reason for hiding this comment

paddycarver Oct 2, 2018

Choose a reason for hiding this comment

paddycarver commented Oct 2, 2018

danawillow left a comment

Choose a reason for hiding this comment

paddycarver commented Oct 2, 2018

paddycarver commented Oct 2, 2018

ghost commented Nov 16, 2018