Terraform Test: cleanup command

[dsa0x]

Fixes #

Target Release

1.13.x

CHANGELOG entry

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

[liamcervante]

Nice! I like how you managed to reuse the same nodes from the main test command!

[liamcervante]

Could you add another new test case, similar to this one, where a variable depends on a prior run block. I think this implementation might not work for that case as we won't be able to retrieve the required input variable in that case.

We might need to preserve the input variables for each run block in the manifest file as well so we can retrieve them, or even have to execute the earlier run blocks to get the input variables again in the case of ephemeral or sensitive vars.

We could also look into this in a future PR before launch.

[dsa0x]

I see, that raises an important point worth documenting. However, It might feel a bit unintuitive for users that we execute runs when trying to clean up the state.

Do we even need the variables for a destroy plan? Besides provider config, The required inputs for destruction are probably already computed. Maybe we could provide just the state and an empty config to the plan. That should be enough to destroy everything defined in the state, right?
The striken-out text just feels like it will lead to other complication

[liamcervante]

Terraform requires values for all non-optional input variables in all plan operations (normal, refresh, and destroy). It only actually uses the ones that end up in provider configurations but it doesn't know up front which ones are needed for what so it just requires all of them.

The same problem would be true for provider configs in the test file as well. If we need to initialise any providers in order to run the destroy, they might refer to outputs of previous test runs which will need to be recomputed.

[dsa0x]

You're right that the non-optional values are needed regardless.
I still have some doubts about the new command executing runs. Is there a clear distinction between what it does and the normal test command then?

In the new command, we will have to also execute the runs, and then cleanup after them. This is something that we do in the normal mode anyway.

Perhaps one way to see it is that the new command is just a way to execute runs without reporting their failure. If that is the case, then should it be a flag?

Also, the new command cleanup might fail and leave new state behind, but I guess you can simply rerun it

[dsa0x]

We might need to preserve the input variables for each run block in the manifest file as well so we can retrieve them

I doubt that this is sufficient, because the references in subsequent blocks may be a computed output. For that reason, prior run execution makes better sense.

Nevertheless, that is not without problems. For example, if a prior run continue to fail, then we can never clean up the state that depends on it. Perhaps that is acceptable?

[dsa0x]

Maybe the problem is that we are trying to shoehorn the cleanup workflow along with the main test workflow. Should there be clearer separation, and a different way to provide input variables for the destroy plan of the left-over state?

[liamcervante]

This is probably complicated enough that we could discuss it offline? We can then resolve the variable question in a follow up PR prior to launch?

[dsa0x]

Sounds good