Skip to content

Conversation

@dsa0x
Copy link
Member

@dsa0x dsa0x commented Apr 17, 2025

The important changes here

  • The config inspection of skip_cleanup is now done in the config file
    • This led to some minor test changes
  • The cleanup nodes are now in a subgraph, so that we are not adding weird dynamic nodes and complicated edges into the graph.
  • After a destroy, we only update the in-memory state resources, and we leave the outputs
  • We now always write the state outputs to file, even after a destroy of resources. This way, if a run1 state's resources has been destroyed by a later run run3, run2 which references run1 can still access the output values, and use as input to its operations.

Target Release

1.13.x

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@dsa0x dsa0x force-pushed the sams/cmd-test-cleanup branch 2 times, most recently from f45f122 to d9adf58 Compare April 17, 2025 12:20
@dsa0x dsa0x added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Apr 17, 2025
@dsa0x dsa0x marked this pull request as ready for review April 17, 2025 12:20
@dsa0x dsa0x requested a review from a team as a code owner April 17, 2025 12:20
@dsa0x dsa0x force-pushed the sams/cmd-test-cleanup branch from d9adf58 to 9c6189e Compare April 18, 2025 07:10
Copy link
Contributor

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

I think we also want to update the normal cleanup operation as well right? If a run block fails to get destroyed, any dependent run blocks that have different state should not be destroyed as we might need the outputs from that state file?

@dsa0x
Copy link
Member Author

dsa0x commented Apr 22, 2025

I think we also want to update the normal cleanup operation as well right? If a run block fails to get destroyed, any dependent run blocks that have different state should not be destroyed as we might need the outputs from that state file?

@liamcervante this would be a change in behavior though. It would imply that a state clean up failing prevents other states from being cleaned up. But you are right, it might be necessary, in order to appropriately cleanup that failed state.

@liamcervante
Copy link
Contributor

Yes, I think it's required though - we need to keep dependent states in place otherwise the cleanup command just can't work. There's no breaking changes so I think its fine. The existing behaviour is just "do nothing", so this change being introduced alongside the state manifest file means the user is being left with a lot more information now.

@dsa0x dsa0x force-pushed the sams/cmd-test-cleanup branch from 2026f29 to 100f866 Compare April 23, 2025 09:25
@dsa0x dsa0x force-pushed the sams/cmd-test-cleanup branch 2 times, most recently from 855a272 to 1cfc4f0 Compare April 23, 2025 18:15
@dsa0x dsa0x force-pushed the sams/cmd-test-cleanup branch from 1cfc4f0 to bb14136 Compare April 23, 2025 18:15
@dsa0x
Copy link
Member Author

dsa0x commented Apr 23, 2025

The important changes here

  • The config inspection of skip_cleanup is now done in the config file
    • This led to some minor test changes
  • The cleanup nodes are now in a subgraph, so that we are not adding weird dynamic nodes and complicated edges into the graph.
  • After a destroy, we only update the in-memory state resources, and we leave the outputs
  • We now always write the state outputs to file, even after a destroy of resources. This way, if a run1 state's resources has been destroyed by a later run run3, run2 which references run1 can still access the output values, and use as input to its operations.

@dsa0x dsa0x requested a review from liamcervante April 23, 2025 18:24
Copy link
Contributor

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

Looks good! Just super nit on file naming.

@dsa0x dsa0x merged commit 6f4dea3 into f-controlling-destroys Apr 24, 2025
8 checks passed
@dsa0x dsa0x deleted the sams/cmd-test-cleanup branch April 24, 2025 08:32
@github-actions
Copy link
Contributor

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 May 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants