Terraform Test: Allow skipping cleanup of entire test file or individual run blocks #36729
Conversation
dsa0x
added
the
no-changelog-needed
dsa0x
force-pushed
the
sams/test_skip_cleanup
branch
from
f121f50 to
88896d6
Compare
dsa0x
force-pushed
the
sams/test_skip_cleanup
branch
from
88896d6 to
7f4cb14
Compare
| stateGetter := func(stateKey string) (*states.State, error) { | ||
| backend := local.New() | ||
| dir := filepath.Dir(stateKey) | ||
| backendConfig := cty.ObjectVal(map[string]cty.Value{ | ||
| "path": cty.StringVal(stateKey), | ||
| "workspace_dir": cty.StringVal(dir), | ||
| }) | ||
| diags := backend.Configure(backendConfig) | ||
| if diags.HasErrors() { | ||
| return nil, fmt.Errorf("failed to configure backend: %s", diags) | ||
| } | ||
|
|
||
| mgr, err := backend.StateMgr("default") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create state manager: %s", err) | ||
| } | ||
| if err := mgr.RefreshState(); err != nil { | ||
| return nil, fmt.Errorf("failed to refresh state: %s", err) | ||
| } | ||
| return mgr.State(), nil |
There was a problem hiding this comment.
I've been learning about the state packages recently, and an easier way to implement code that does this is using a statemgr.Filesystem:
// stateKey is value like ".terraform/test/TpQOqtev.tfstate"
sm := statemgr.NewFilesystem(stateKey)
err := sm.RefreshState()
if err != nil {
return nil, fmt.Errorf("error when reading state file: %w", err)
}
return sm.State(), errAlternatively, I think your helper can be replaced with the pre-existing testStateRead function! I've just been refactoring some code to remove my own implementation of this helper also, and it looks like there are a number of helpers like this in the codebase 😂
There was a problem hiding this comment.
Left a few comments:
- Some nit-like comments about how terminal output could make it clearer about whether cleanup is being skipped, and why.
- A comment about some scenarios with conflicting skip_cleanup instructions that it might be useful to add tests for.
In the case of conflicting skip_cleanup instructions from run blocks versus the test block, should that cause a parsing error or should the test run and during cleanup users see a warning about the conflicting instructions?
| run "test_three"... pass | ||
| run "test_four"... pass | ||
| run "test_five"... pass | ||
| main.tftest.hcl... tearing down | ||
| main.tftest.hcl... pass |
There was a problem hiding this comment.
In the happy path (i.e. when the warning isn't present in the terminal output) would it be beneficial to indicate that the tearing down process is affected by skip_cleanup?
E.g.
run "test_five"... pass
main.tftest.hcl... tearing down
+ cleanup stopped at run "test_three" due to use of skip_cleanup
main.tftest.hcl... pass| run "test_five"... pass | ||
| main.tftest.hcl... tearing down | ||
| main.tftest.hcl... pass |
There was a problem hiding this comment.
If teardown is prevented for the entire file through the test block, is that information available early enough to affect this terminal output? A user seeing that output might think that the skip_clean up wasn't working, until they check state.
E.g.
- main.tftest.hcl... tearing down
+ main.tftest.hcl... tear down skipped for this file
main.tftest.hcl... passor
main.tftest.hcl... tearing down
+ clean up is skipped for this file
main.tftest.hcl... pass| @@ -2214,6 +2214,373 @@ Success! 5 passed, 0 failed. | |||
| } | |||
| } | |||
|
|
|||
| func TestTest_SkipCleanup(t *testing.T) { | |||
There was a problem hiding this comment.
What's the expected behaviour when a user sets skip_cleanup = true in the test block and also in an individual run block?
Similarly, what's the expected behaviour when a user sets skip_cleanup = false in the test block and skip_cleanup = true in an individual run block? Does the user receive a warning from these two conflicting instructions?
Implements
skip_cleanup, which allows skipping the cleanup of individual run blocks or the entire test file. This simply ensures that the cleanup stage for the state is an apply of the run requesting the skip, instead of the normal destroyTarget Release
1.12.x
CHANGELOG entry