terraform-provider-local icon indicating copy to clipboard operation
terraform-provider-local copied to clipboard

add support for lifecycle ignore_changes

Open tkellen opened this issue 4 years ago • 14 comments

This makes it so it is possible to ignore changes to content or content_base64. The previous read implementation was a bit too naive.

Is it necessary to write tests for this given that this modified implementation relies on upstream lifecycle.ignore_changes diffing behavior?

tkellen avatar Dec 29 '19 21:12 tkellen

Gentle nudge for a review?

tkellen avatar Jan 09 '20 17:01 tkellen

ping @terraform-providers/enablement

tkellen avatar Jan 21 '20 01:01 tkellen

hail mary pass, ping @appilon @apparentlymart @radeksimko?

tkellen avatar Jan 25 '20 15:01 tkellen

even harder hail mary pass. ping @mitchellh?

tkellen avatar Feb 04 '20 13:02 tkellen

HELP! HELP! THERE IS A FIRE IN THE BUILDING!

...just kidding, but is anyone out there?

tkellen avatar Feb 19 '20 07:02 tkellen

help is coming, I know it is! castaway

tkellen avatar Feb 20 '20 03:02 tkellen

Would it help to push a commit to fix the travis linting error?

tristanmorgan avatar Feb 20 '20 04:02 tristanmorgan

Did that. Looks like there are some failing test cases as well. I'd be happy to invest the time in fixing them if anyone who has the ability to actually approve this PR would chime in on if that has a chance of happening.

tkellen avatar Feb 20 '20 06:02 tkellen

Thanks for the PR, @tkellen. I appreciate the work on the Read implementation and agree that this is the right direction to go in. There's a bit of overlap here with https://github.com/terraform-providers/terraform-provider-local/pull/26, but ultimately https://github.com/terraform-providers/terraform-provider-local/pull/26 does not fix the ignore_changes issue. We can merge this PR once we have the following:

  • Test case added for ignore_changes in config. While you're right that we do rely on Core's implementation of lifecycle.ignore_changes, the peculiarities of this provider make it easy to make mistakes that cause ignore_changes to break. Adding a test case ensures future contributions do not break this functionality.

  • Other test cases fixed. I haven't looked into this in detail, but please add a comment if you're not sure how to fix these. I will shortly be updating this provider to version 1.7.0 of the plugin SDK and enabling the new binary acceptance test driver, which should iron out discrepancies between provider acceptance tests and manual tests using the Terraform CLI. You can track this at https://github.com/terraform-providers/terraform-provider-local/issues/38 and rebase your branch once it's merged into master if you'd like to test this out.

I'd like to add respectfully that adding "nudge" comments to PRs will not change the timeline in which the maintainers of this provider will review them. While waiting for a PR, you're welcome to update it with more information that will help us triage the issue, or fix linting and test failures.

kmoe avatar Feb 20 '20 21:02 kmoe

Thanks for the response @kmoe! I'll ping back here when I've had time to address the additions you've described.

With respect to the nudge comments, I see PRs that are 1+ year old on this repository that nobody ever posted on a comment on. It's hard to see from an outsiders perspective what triggered your review of this other than my silly commentary (I hope it was obviously playful and not incendiary). Do ya'll have any documentation about what a contributor should expect in terms of timeline? For example, if I knew you only reviewed this provider quarterly or something I would change my approach considerably.

tkellen avatar Feb 21 '20 00:02 tkellen

@kmoe @tkellen Any update on the status of this? Until this is added so that ignore_changes works, we've been unable to find a way to create a bunch of files on bootstrapping and then keep most but not all of them up to date via terraform. The use case here is that there's a bunch of boilerplate files we want to keep standardized via terraform but there's also a few config files that we expect to be customized but don't want people to have to manually create the first time.

@kmoe @appilon as far as the test failing, that looks to be related to the repo having changed organizations and https://github.com/hashicorp/terraform-provider-local/pull/40 so I think that needs to be changed on the hashicorp side. Honestly it is quite confusing that the provider lives here instead of in terraform-providers organization with everything else

vs-jawad avatar Apr 16 '20 19:04 vs-jawad

Sounds like your use-case is exactly the same as mine @vs-jawad. Unfortunately no, I haven't been able to dig into this further just yet. I wound up going with a shell script / envsubst approach due to the delay in feedback here and the fire that was burning on this is now out.

tkellen avatar Apr 16 '20 19:04 tkellen

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


Tyler Kellen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you already have a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

hashicorp-cla avatar Mar 12 '22 17:03 hashicorp-cla

is this at all being considered, happy to give it a shot myself it this PR has been abandoned.

hiveposse avatar Jan 11 '24 14:01 hiveposse