fix error when writing file with empty content

[kmoe]

Fixes #15

Previously, attempting to write a local file with content of "" would set the resource Id to "", since the Id is based on a SHA1 of the content. This caused the file to be deleted during the apply step.

The resource Id of an empty file is now the SHA1 of the filename concatenated with the SHA1 of the content, so it will never be "".

[radeksimko]

Do you mind attaching a test case which reproduces this behaviour (i.e. fails without a patch)?

Side note: We have @terraform-providers/enablement alias that can be used when requesting reviews from the whole team 😉

[kmoe]

Thanks @radeksimko! Apologies for the premature review request - I'll fix the tests and add a new case for this.

[apparentlymart]

The id isn't really meaningful for this resource type (it gets all of its required information from the other attributes) so if we're going to change it anyway, maybe more straightforward to just set it to some constant value like "-" to make it clearer that it's not significant, and then we can remove it altogether once a later SDK version removes the requirement for having an id attribute. (This requirement is now an SDK-only thing; Terraform Core just treats it the same as any other attribute.)

I'm not sure if this is important, but note that anyone that was referencing local_file.example.id and expecting to get a SHA1 checksum will see this as a breaking change. I think we never documented this as possible, so probably not a big deal, but probably worth mentioning in the changelog once this is merged just in case.

[kmoe]

It turns out the issue in #15 no longer causes a problem for the user as of Terraform 0.12: #15 (comment)

Instead, a warning is logged because the plan ends up with a null value for the content attribute where it should have "". The state file also ends up with "content": null. However, since these are considered equivalent in 0.12 for the purposes of SDK diffing (https://github.com/hashicorp/terraform/blob/master/config/hcl2shim/values_equiv_test.go#L39,L41), the user does get an empty file written with no errors.

Fix

I've fixed this issue in c507fd8, and manual tests pass for Terraform 0.11.13 and 0.12 latest.

With this patch, no warning is logged, and the contents of the final state file are valid (content: "") in both 0.11.13 and 0.12.

Tests

I haven't managed to make an acceptance test case that fails before this patch.

Since this provider is already using the 0.12 SDK, the code path followed when running terraform apply under 0.11 is significantly different from that when running make testacc. I don't think any action is required here.

However, the code path followed when running terraform apply under 0.12 involves EvalDiff.Eval, which is what produces the warning (https://github.com/hashicorp/terraform/blob/master/terraform/eval_diff.go#L210). It would be helpful to expose this test in the acceptance test framework for providers. I don't see a way to do this currently but would appreciate feedback from @apparentlymart to check my assumptions here.

In the meantime I've added an acceptance test inhttps://github.com//pull/26/commits/c507fd81316fa8ea99ec9d4498522e3b004c6853 which doesn't currently fail before the patch. I'll update this or at least add a comment after discussion.

[apparentlymart]

The test framework calls in to Terraform through terraform.NewContext and should be running all of the same steps as normal CLI usage from that point onwards, so I'm surprised that EvalDiff codepath isn't being exercised. 🤔

I wonder if some other detail about the test framework is changing the outcome here. terraform apply internally runs a validate, a refresh, a plan, and then an apply walk. The test harness follows a similar pattern but does some of the steps multiple times to ensure the provider is being consistent. Off the top of my head I can't think of a reason why that would change the outcome of the particular test cases you added there, but we have seen that be a cause occasionally before, and I don't have any other immediate ideas.

The testing process is implemented in testStep; you can see there it calling a few different operation methods on a terraform.Context. The corresponding main code is in the local backend's terraform apply implementation. You can see there that both of them are calling various methods on the terraform.Context object, but the test harness does a few extra calls.

[BrendanThompson]

A point here, there are scenarios wherein one may want to create a file but not care if its edited in the future. With the current implementation it is not possible to do this even with ignore_changes due to the content checksum for the Id.
This change, however, looks as though it will partially solve this issue; however, the resourceLocalFileRead will still nuke the Id if the content doesn't match.
What would you ( @apparentlymart ) recommend here, it is unclear to me as of yet if simply using an ignore_changes on the content attribute will fix this particular scenario now that the Id is no longer reliant on the content hash.

[hashicorp-cla]

All committers have signed the CLA.