Fix google_compute_disk created from snapshot, forces new resource then apply once more

[z1nkum]

fix issue with recreation loop:
#218

[paddycarver]

Hi @z1nkum! Thanks so much for the PR. Overall, this solution seems like it would work, but I think there's a way we could do this that has a few nicer properties.

If we use a DiffSuppressFunc on the snapshot property, it lets us store the full URL of the snapshot, but doesn't consider it different from just the name being used in the config.

If the DiffSuppressFunc uses a similar Split strategy on what's in the config, as well, then the config can accept either the full URL or the name, which is sometimes beneficial.

What do you think of these two changes? If you have questions or need help, I'm happy to assist however I can. And thanks for contributing to Terraform!

[z1nkum]

@paddycarver please, take a look. Maybe I did not quite understand the idea.

[paddycarver]

This is great @z1nkum, looks perfect!

Last thing I see before this can be merged is just adding a test case that fails without the fix but passes with the fix. google/resource_compute_disk_test.go should have plenty of examples you can use as a template, but if you get lost or confused, please don't hesitate to shout and I'll help you get this pushed over the finish line.

Super excited to have this fix in Terraform. Thanks!

[z1nkum]

I really tried to figure out how to trigger and then catch ForceNew event in tests - but unsuccessfully
Test TestAccComputeDisk_fromSnapshotURI (google/resource_compute_disk_test.go) looks like suitable for this case. Perhaps we should add here one more resource.TestStep with the same config. But I can not imagine what should be inside ComposeTestCheckFunc test :(
Could you please help me with it, @paddycarver ?

I've already built provider with fix and check against config (#218) - works fine (at least, does not attempt to recreate the disk)

[paddycarver]

Hey @z1nkum, absolutely no worries. There are a lot of tools in the test framework, and it's not documented as well as it could be.

In this case, I don't think the bug is that it's ForceNew--and that doesn't look like it's changing in your code. So I'd forget about trying to verify the ForceNew aspect of it, and rather just try to verify that there isn't a diff after the apply.

Fortunately, the test framework does that for us automatically; it will run another plan after all your test steps are run, and if there's a diff, it fails the test. So a good way to test this would be to use the snapshot name (the input that was causing the problem) in a test. If the test passes, things are good, and that's all it takes! If you want to be extra sure, run the test against the master branch (without the fix applied) and make sure it fails, with something like "non-empty plan" or "non-empty diff". Then you know you've recreated the problem, and fixed it.

You could even copy & paste the current snapshot test (https://github.com/terraform-providers/terraform-provider-google/blob/ff8bdc9b52f379b7cb42a2c2fe6dcb49e0c130c3/google/resource_compute_disk_test.go#L63-L85) and update the copy to use just the name, instead of the full URL. That would demonstrate that the problem was resolved. And having it automated means if the API ever changes, or we ever break Terraform's framework in a way that breaks this, or we ever introduce a regression, we'll know about it. :)

Thanks for all the hard work!

[z1nkum]

Thank you, @paddycarver - I've got it.

I wrote test (actually, update TestAccComputeDisk_fromSnapshotURI) and tested it against master/branch. As expected, test passed on branch and fail on master:

--- FAIL: TestAccComputeDisk_fromSnapshot (152.56s)
        testing.go:428: Step 1 error: After applying this step, the plan was not empty:

[z1nkum]

😱 sorry, I just wanted to rebase branch from upstream/master

[paddycarver]

No worries! I've created a fixed branch (terraform-providers:paddy_z1nkum_238) that is up-to-date with master. You can:

• pull that, mirror it to your repo, and use it as the basis of the PR
• pull that, force push it to the z1nkum:snapshot-fix-url-to-name branch you opened this PR from, which will update this PR
• just redo the steps I did to make that branch on your branch and force push it, which will update this PR:
  1. git checkout snapshot-fix-url-to-name
  2. git checkout d0b7fbc
  3. git merge master
  4. git push -f

Note that using the branch I created or the steps above will both delete anything you've done on the branch since d0b7fbc, which (as far as I can tell) is the last intentional commit. :)

[z1nkum]

Thanks a lot! Now it seems all right

[paddycarver]

I think the way the ref selector was done for the test isn't ideal, and would probably be better as something like a boolean. That being said, I don't think it's a big enough issue to hold this fix over, so I'd say let's merge it. :)

Thanks for the hard work, the patience, and for contributing to Terraform! Really appreciate you tackling this bug for us. Great job!

[ghost]

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!