Remove cache

[meyskens]

This removes the internal cache system by passing JSON strings as
rendered property to the data resource. This gets passed to
ignition_config instead of the ID.
This uses the Terraform state as cache instead of the old in memory module.

This change is breaking as it will need to have all ID references replaced.

Signed-off-by: Maartje Eyskens maartje@eyskens.me

[alexsomesan]

I'm fine merging this as mitigation since there is limited capacity right now to investigate the full chain of events that causes #57.

[meyskens]

@alexsomesan thx for the heads up! I will try to finish this PR (mostly tests and cleanup) so we can move on!

[meyskens]

I changed all tests to no longer use IDs, ready for review now!

[seh]

Given that we're dead in the water trying to upgrade to Terraform version 0.12 until we can get Ignition working, I'm interested in two things first here:

• What do you mean by "[this change] is breaking as it will need to have all ID references replaced?"
  Replaced where? Do you mean that Terraform will consider every item contributing to the rendered Ignition JSON to have changed?
• What is holding up merging this patch?

[meyskens]

@seh the breaking change is best illustrated in the tests:
https://github.com/terraform-providers/terraform-provider-ignition/pull/56/files#diff-ce99dcb7b0e71397a93e01fe364c3f41R97
Instead of referring to IDs now actual json is passed

Currently waiting on somebody to review this to get it merged.
cc @alexsomesan

[seh]

Thank you for the explanation. Now I see clearly how you changed how we knit together these Ignition fragments.

It’s an unusual approach in the context of Terraform providers. What makes it difficult for a provider to maintain an identifier-to-object mapping like this? Does, say, the “aws” provider get away with it more easily because the mapping is stored externally by AWS? #50 proposes moving the mapping/cache out of memory onto the local disk, presumably to handle the problem of the provider process getting killed and restarted one or more times by Terraform.

Is there documentation that needs to change as well to drop mentioning the “id” field in favor of the “rendered” field?

[meyskens]

Storing it to disk could also be a solution but gave some issues for me in the past. Keeping it as rendered somehow also stores it to disk but in the state file.

And you're right while testing things out I totally forgot the documentation changes!

[seh]

I built, installed, and tried using this version of the provider. It required replacing about 25 occurrences of "id" with "rendered" in my Terraform configuration, but once I did that, I found that it worked with Terraform version 0.12.7.

I'll perform more ambitious testing later today and tomorrow, but for now I'll say that though the change in the referring attribute name is an unfortunate trade, it's worth it to have a properly functioning provider. Thank you, @meyskens!

[seh]

I've since tested this more comprehensively, and it's working well for us.

@alexsomesan, you mentioned about eight weeks ago that you'd be willing to merge this. Can we proceed? Without this fix merged and released, it's difficult to migrate to Terraform 0.12. Every member of the team would need to set up a home-built provider in the ~/.terraform.d/plugins tree.

[smalltown]

I tested this PR in Terraform 0.12.8, everything works fine like @seh said, @alexsomesan could you help to take a look for this PR and merge it, thanks ^^

[seh]

As I've tested this more, I now have a concern: It seems that my "user_data" fields to my EC2 instances and launch configurations change every time that I use terraform plan after terraform apply.

I'm going to capture the user data strings between two runs to see if indeed the Ignition provider is now generating something that varies with each run, despite the input not changing.

[seh]

Well, comparing the resulting "user_data" values—both the raw value as compact JSON, and then run through jq—reveal no differences, yet every time I run terraform plan, Terraform says that the user data value is forcing replacement of the "aws_instance" resources. The same goes for my launch configurations.

I'll try to look into this more over the weekend.

[seh]

After reading several Terraform GitHub issues on the commute home (search for, say, "Terraform user_data force" and take a deep breath), I discovered that I've had a depends_on attribute in my ignition_file data sources, expressing a dependency explicitly on the related aws_s3_bucket_object resource, with this comment in my code explaining why from about a year and a half ago:

Work around Terraform defect with data sources depending on resource attributes, in which it fails to update the former when the latter changes.

Removing that depends_on attribute today allows terraform plan to relent on finding spurious changes in the "user data" content. What I don't know yet, though, is whether the data source will properly reflect changes in the S3 object content. We're now trying this with a much evolved Terraform version, which I hope has since fixed that defect.

To back up from my accusation earlier, I don't think this is a problem with this provider. I hope to get in more testing over the weekend to confirm that I was able to work around this spurious change problem.

[lipsa-vlad]

I guess this will also fix #55.

[seh]

Yes, I think it does fix that one. My configuration uses both, and works with this provider.

Looking at other recent committers here besides @alexsomesan, there's @appilon and @pbrit. Would either of you two be able to help us move forward with this patch?

[lipsa-vlad]

I have tested this ignition fix using terraform v0.12.9-dev and it worked as expected. Only change I had to do was to replace id with rendered in ignition_config.

[sdlarsen]

This PR fixes ignition woes for me. Would be nice to see it merged.

[smalltown]

Hi @alexsomesan and @appilon

Sorry to bother you,
This PR has tested by many people,
and it's a blocker when upgrading Terraform from 0.11 to 0.12,
Could you please kindly review and merge it,

Thank for considering my request

[esomore]

Any progress on this one?

[alexsomesan]

LGTM. Acceptance tests pass.
I'm also basing this decision on multiple user reports in the comments stating that the change works in their contexts.

[seh]

@alexsomesan, for us to get the full value from this patch, we'll need a fresh release for the provider. What's involved in issuing a release?

[seh]

This change is now available in release 1.2.0.