terraform: destroy node should not create

[mitchellh]

/cc @catsby @phinze: This is a doozy, but is a good example of the new refactor and how it comes into play in fixing bugs.

@catsby ran into a "diffs not matching" error on an acceptance test. Little did I know it'd lead me down this rabbit hole. Basically what was going on was that the acceptance test did a create before destroy (wow!) and in create before destroy, the destroy nodes were actually also doing the destroy. Shockingly, the Terraform core tests still passed. Still looking into how to make that not happen... but in the mean time, this PR.

I want to note that this bug was entirely introduced by the refactor merge, and is therefore not present in 0.3.7. This is just a master-only bug.

Here are the major changes made in this:

• Destroy nodes now call a new eval node EvalFilterDiff to filter out only the destroy part, and not the creation part. This is the critical change.
• Destroy nodes now call a new EvalRequireState to not call Apply on the provider if there isn't even a state to destroy. This is an optimization but some core tests depend on this behavior. Fair enough.
• Regular nodes during apply now call EvalWriteDiff with a nil diff. This fixes an issue where during create-before-destroy, the creation was keeping the diff around (not writing nil diff), and the destroy node was doing a destroy then re-create. I feel this logic is more correct anyways: the diff is gone once the apply is done.

More comments inline in the code to better explain.

[mitchellh]

This change allows us to EvalWriteDiff a nil value.

[mitchellh]

Unrelated CI errors... pass locally:

terraform b-destroy-not-create → make
go generate ./...
TF_ACC= go test ./...  -timeout=30s -parallel=4
ok      github.com/hashicorp/terraform  0.017s
ok      github.com/hashicorp/terraform/builtin/bins/provider-atlas  0.019s
ok      github.com/hashicorp/terraform/builtin/bins/provider-aws    0.017s
ok      github.com/hashicorp/terraform/builtin/bins/provider-cloudflare 0.043s
ok      github.com/hashicorp/terraform/builtin/bins/provider-cloudstack 0.016s
ok      github.com/hashicorp/terraform/builtin/bins/provider-consul 0.020s
ok      github.com/hashicorp/terraform/builtin/bins/provider-digitalocean   0.015s
ok      github.com/hashicorp/terraform/builtin/bins/provider-dnsimple   0.016s
ok      github.com/hashicorp/terraform/builtin/bins/provider-google 0.018s
ok      github.com/hashicorp/terraform/builtin/bins/provider-heroku 0.019s
ok      github.com/hashicorp/terraform/builtin/bins/provider-mailgun    0.021s
ok      github.com/hashicorp/terraform/builtin/bins/provider-null   0.016s
ok      github.com/hashicorp/terraform/builtin/bins/provisioner-file    0.015s
ok      github.com/hashicorp/terraform/builtin/bins/provisioner-local-exec  0.015s
ok      github.com/hashicorp/terraform/builtin/bins/provisioner-remote-exec 0.016s
ok      github.com/hashicorp/terraform/builtin/providers/atlas  0.015s
ok      github.com/hashicorp/terraform/builtin/providers/aws    0.021s
ok      github.com/hashicorp/terraform/builtin/providers/cloudflare 0.014s
ok      github.com/hashicorp/terraform/builtin/providers/cloudstack 0.018s
ok      github.com/hashicorp/terraform/builtin/providers/consul 0.015s
ok      github.com/hashicorp/terraform/builtin/providers/digitalocean   0.014s
ok      github.com/hashicorp/terraform/builtin/providers/dnsimple   0.013s
ok      github.com/hashicorp/terraform/builtin/providers/google 0.018s
ok      github.com/hashicorp/terraform/builtin/providers/heroku 0.018s
ok      github.com/hashicorp/terraform/builtin/providers/mailgun    0.016s
ok      github.com/hashicorp/terraform/builtin/providers/null   0.014s
ok      github.com/hashicorp/terraform/builtin/provisioners/file    0.016s
ok      github.com/hashicorp/terraform/builtin/provisioners/local-exec  0.030s
ok      github.com/hashicorp/terraform/builtin/provisioners/remote-exec 0.015s
ok      github.com/hashicorp/terraform/command  0.580s
ok      github.com/hashicorp/terraform/config   0.093s
ok      github.com/hashicorp/terraform/config/lang  0.010s
ok      github.com/hashicorp/terraform/config/lang/ast  0.009s
ok      github.com/hashicorp/terraform/config/module    5.191s
ok      github.com/hashicorp/terraform/dag  0.010s
ok      github.com/hashicorp/terraform/depgraph 0.018s
ok      github.com/hashicorp/terraform/digraph  0.013s
ok      github.com/hashicorp/terraform/flatmap  0.008s
ok      github.com/hashicorp/terraform/helper/config    0.015s
ok      github.com/hashicorp/terraform/helper/diff  0.017s
ok      github.com/hashicorp/terraform/helper/hashcode  0.008s
ok      github.com/hashicorp/terraform/helper/multierror    0.011s
ok      github.com/hashicorp/terraform/helper/resource  1.363s
ok      github.com/hashicorp/terraform/helper/schema    0.032s
ok      github.com/hashicorp/terraform/helper/ssh   0.357s
ok      github.com/hashicorp/terraform/helper/url   0.007s
ok      github.com/hashicorp/terraform/plugin   0.251s
ok      github.com/hashicorp/terraform/rpc  0.066s
ok      github.com/hashicorp/terraform/state    0.038s
ok      github.com/hashicorp/terraform/state/remote 3.460s
ok      github.com/hashicorp/terraform/terraform    0.824s
go tool vet -asmdecl -atomic -bool -buildtags -copylocks -methods -nilfunc -printf -rangeloops -shift -structtags -unsafeptr .

[mitchellh]

Also, @catsby's acceptance test now passes.

[mitchellh]

I'm going to merge this now because this fixes a pretty critical bug and I don't want anyone running master to run into it.

[catsby]

[phinze]

@mitchellh We've talked about this a bit, and I know you were flying forward on a critical bug here so I'm not talking about this specific instance, but for new test case harnesses can we agree to prefer:

• Maps with test names instead of integer indexes
• Failure output that includes test name, expected output, and actual output (instead of just "bad: actual")

Perhaps I am a dummy, but for tests written in this style it takes me a lot of mental ramp time to be able to jump in and read these coherently. My theory is that a few tweaks today can provide compounding savings of future developer time.

[mitchellh]

So I've been thinking about it in each instance, and the main thing for is "does the actual test case provide information vs. what I can provide here" and in this case I figured that the tc.Input and output give you everything you need to fix the bug. If the test itself is wrong then yes, you'd have to count the indices, but I'm assuming the test case itself is correct and won't need to be changed and that if you're fixing an implementation bug then this would catch it.

So, I'd say in this case, option 2 is best.

[phinze]

I figured that the tc.Input and output give you everything you need to fix the bug

Thing is, you don't get input and output... you get something like:

--- FAIL bad: 2

&InstanceDiff{},

As a future human encountering this, I know I broke something, but how am I to go about figuring out what I broke?

If instead, the output was

--- FAIL "preserves Destroy: false"

expected: &InstanceDiff{Destroy: false},
got: &InstanceDiff{},

I immediately know what I broke.

I am attempting to optimize for Cost for Future Human Reloading of this Context into their Brains (CFHRCB). 😀

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.