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

Prevent errors from NoOp deposed changes #31902

Merged
merged 1 commit into from
Oct 3, 2022
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Sep 29, 2022

If a previously deposed object is deleted outside of Terraform, the next plan will result in a NoOp change for the deposed object.

If that NoOp change was flagged as an update to run any associated condition checks, it would fail the following test to ensure a deposed change is only ever participating in a delete operation. Fix the check to verify that the deposed object has an acceptable action rather than use the update flag.

Fixes #31896

@jbardin jbardin added the 1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Sep 29, 2022
@jbardin jbardin requested a review from a team September 29, 2022 14:54
@jbardin jbardin self-assigned this Sep 29, 2022
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable surgical change to fix the bug in the v1.3 branch!

For the main branch I wonder about reverting this and instead changing the logic inside the graph node execute function. I'd prefer for us to (cautiously) trend toward always having a complete graph and then handling the situation inside the normal control flow in the individual graph nodes, because these special exceptions in the graph builder have historically been a bug/regression hazard when adding new features. However, a decision on that need not block moving forward with this tactical fix.

@jbardin
Copy link
Member Author

jbardin commented Sep 29, 2022

That's a good point. The failure here isn't in the graph node, rather the very next block of code. I can easily rewrite this check so it only excludes the that block rather than skipping NoOp node entirely.

If a previously deposed object is deleted outside of Terraform, the
next plan will result in a NoOp change for the deposed object. Fix the
check to verify that the deposed object has an acceptable action rather
than use the `update` flag.
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, whoops... sorry I didn't notice you'd changed this PR to follow the new approach. This also looks good!

@jbardin jbardin merged commit fcec9e2 into main Oct 3, 2022
@jbardin jbardin deleted the jbardin/noop-deposed branch October 3, 2022 16:10
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Invalid planned change for deposed object
2 participants