Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for lifecycle ignore_changes #35

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tkellen
Copy link

@tkellen tkellen commented Dec 29, 2019

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
Copy link
Author

tkellen commented Jan 9, 2020

Gentle nudge for a review?

@tkellen
Copy link
Author

tkellen commented Jan 21, 2020

ping @terraform-providers/enablement

@tkellen
Copy link
Author

tkellen commented Jan 25, 2020

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

@tkellen
Copy link
Author

tkellen commented Feb 4, 2020

even harder hail mary pass. ping @mitchellh?

@tkellen
Copy link
Author

tkellen commented Feb 19, 2020

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

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

@tkellen
Copy link
Author

tkellen commented Feb 20, 2020

help is coming, I know it is!
castaway

@tristanmorgan
Copy link
Member

tristanmorgan commented Feb 20, 2020

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

@ghost ghost added size/S and removed size/XS labels Feb 20, 2020
@tkellen
Copy link
Author

tkellen commented Feb 20, 2020

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.

@kmoe kmoe self-assigned this Feb 20, 2020
@kmoe
Copy link
Member

kmoe commented Feb 20, 2020

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 #26, but ultimately #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 Enable new binary acceptance test driver #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.

@tkellen
Copy link
Author

tkellen commented Feb 21, 2020

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.

@vs-jawad
Copy link

vs-jawad commented Apr 16, 2020

@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 #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

@tkellen
Copy link
Author

tkellen commented Apr 16, 2020

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.

Base automatically changed from master to main February 1, 2021 17:27
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

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.

@hiveposse
Copy link

hiveposse commented Jan 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants