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

Fix google_compute_instance_template crash #3194

Merged

Conversation

c2thorn
Copy link
Member

@c2thorn c2thorn commented Mar 2, 2020

Fixes hashicorp/terraform-provider-google#5018

ForceNew logic for source_image was handled with in a custom diff. When source_image was a self_link to another resource, the custom diff would ForceNew the field before the dependent resource was resolved. This would lead to the SDK trying to sort out a change to a nil value.

To fix this, I took the forcenew logic out of the custom diff, and just set the field to ForceNew in the schema. This way, Terraform will resolve the ForceNew normally. The remaining custom diff acts as a more complicated diffSuppress. I'm not sure if it can be rewritten as an actual diffSuppress, since it takes in the meta object, where diffSuppress does not.

Also added a test that tests this specific scenario since it wasn't covered in the current set of tests.

Release Note Template for Downstream PRs (will be copied)

Fixed a scenario where `google_compute_instance_template` would cause a crash.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 67 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 2 files changed, 67 insertions(+), 9 deletions(-))

@c2thorn c2thorn requested a review from danawillow March 2, 2020 20:39
Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

The original PR to add this (hashicorp/terraform-provider-google#1995) has the note:

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 assume that's all fine and good now?

@c2thorn
Copy link
Member Author

c2thorn commented Mar 2, 2020

The original PR to add this (terraform-providers/terraform-provider-google#1995) has the note:

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 assume that's all fine and good now?

Super interesting, but I don't believe it's a problem now. The diff.clear(key) properly un-sets the ForceNew, and a diff doesn't show.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants