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

plans: indicate when resource deleted due to move #31695

Merged
merged 1 commit into from Aug 30, 2022
Merged

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented Aug 26, 2022

Add a new ChangeReason, ReasonDeleteBecauseNoMoveTarget, to provide better information in cases where a planned deletion is due to moving a resource to a target not in configuration.

This provides the user an additional clue as to what has happened, in a case in which Terraform has elided a user's action and inaction into one potentially destructive change.

Example

Consider a state which contains the resource null_resource.foo.

Configuration

moved {
  from = null_resource.foo
  to = null_resource.nonexist
}

Steps

  1. terraform plan

Before this PR

moved1

After this PR

moved2

@kmoe kmoe changed the title plan: indicate when resource deleted due to move plans: indicate when resource deleted due to move Aug 26, 2022
@kmoe kmoe marked this pull request as ready for review August 30, 2022 10:54
@kmoe kmoe requested a review from a team August 30, 2022 10:55
Add a new ChangeReason, ReasonDeleteBecauseNoMoveTarget, to provide better
information in cases where a planned deletion is due to moving a resource to
a target not in configuration.

Consider a case in which a resource instance exists in state at address A, and
the user adds a moved block to move A to address B. Whether by the user's
intention or not, address B does not exist in configuration.
Terraform combines the move from A to B, and the lack of configuration for B,
into a single delete action for the (previously nonexistent) entity B.
Prior to this commit, the Terraform plan will report that resource B will be
destroyed because it does not exist in configuration, without explicitly
connecting this to the move.

This commit provides the user an additional clue as to what has happened, in a
case in which Terraform has elided a user's action and inaction into one
potentially destructive change.
Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

The UI looks good to me.

I think we could achieve this same UI result without adding a new change reason. Essentially, taking the logic from NodePlannableResourceInstanceOrphan and moving it to the diff renderer. I'm not sure if that's better or worse, but it'd be a smaller diff.

@kmoe
Copy link
Member Author

kmoe commented Aug 30, 2022

Thank you. I agree that this could be done in principle in the diff renderer. It would require a bit of refactoring which I think would make the code less clear and harder to maintain, since we would have to add logic to say that in this one particular case, the # (because... text is dependent on something other than the ActionReason.

@kmoe kmoe merged commit dec48a8 into main Aug 30, 2022
@kmoe kmoe deleted the kmoe/moved-deleted branch August 30, 2022 17:01
@github-actions
Copy link

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

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 Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants