Skip to content

terraform test: refactor manifest file for simplicity #37412

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

Open
wants to merge 9 commits into
base: liamcervante/f-controlling-destroys
Choose a base branch
from

Conversation

liamcervante
Copy link
Member

This PR aims to address the second of Sarah's concerns from the source branch. Namely, that the state files and manifest file are still be written even if they are empty / didn't error.

I did find the existing implementation of the manifest file to be confusing. States were being loaded and written at seemingly random times, and there was overlap between when things were being written into backends and into local files. There are two driving changes in this PR that I think simplify things a little bit:

  1. The presence of an entry in the manifest file is now enough for the manifest file to know directly whether a state is empty or not. Empty states will simply not be written to the manifest, so the we don't need to load the state outside the graph to try and decide whether we can execute a test or not. If the manifest file has states for a given run in it, then that run needs to be cleaned up before it can execute. We don't need to parse the state files to know this.
  2. The manifest file itself is no longer provided directly into the test graph. Instead, the states are loaded and saved externally, with the graph just needing to be keep them updated in memory. The graph isn't then sometimes writing data into backends, sometimes not, sometimes writing things to files, etc. This all happens now in a single place after the file has finished executing.

This change did end up being larger than I expected, as I needed to move things around in quite a large number of places in order to get the kind of access needed to make the state loading work as intended. For that, I'm sorry. But, hopefully most of the smaller changes are just simple refactorings that you can understand.

There is also one behaviour change. When a run block with a backend fails to get destroyed, an entry is written into the manifest for that state file but it is no longer directly persisted to disk. Instead, the state as it exists with the failed destroy is written directly into the actual state backend. So, we have an entry in the manifest locally telling us the execution failed, but we don't keep duplicate copies of the state file around. Instead, during cleanup, the errored state is just loaded from the backend like normal instead of trying to sideload it locally. It seemed redundant and dangerous to hold competing copies of a state file in two locations.

@liamcervante liamcervante requested a review from a team as a code owner August 6, 2025 14:44
@liamcervante liamcervante added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Aug 6, 2025
Copy link
Member

@SarahFrench SarahFrench left a comment

Choose a reason for hiding this comment

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

Thanks Liam - from a read-through of the diffs these changes look like they simplify things a lot. Could you please address the errors that are stopping the tests from running?

Error: ../../moduletest/graph/transform_state.go:114:23: t.StateManifest undefined (type *TestStateTransformer has no field or method StateManifest)
Error: ../../moduletest/graph/transform_state.go:126:4: t.EvalContext undefined (type *TestStateTransformer has no field or method EvalContext)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

3 participants