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

feat: add state_alias for test run blocks #36185

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kevineor
Copy link

@kevineor kevineor commented Dec 10, 2024

While working on terraform module testing, i've been in need for a test case that ensure that a module upgrade is possible. Tests runs can only plan, apply and destroy a complete new infrastructure. My proposal allows to have multiple run blocks that targets the same infrastructure.

This could allow to make module upgrade tests, by ensuring the second apply using the new code still pass, or even checking that ressources does not get recreated.

Example code i want to implement :

run "old_code" {
  state_key = "module_test"

  variables {
    input1 = "toto"
  }

  module {
    source = "./test/old_code_path"
  }
}

run "new_code" {
  state_key = "module_test"

  variables {
    input1 = ""
  }

  module {
    source = "./test/new_code_path"
  }
}

Fixes #35080

PR Modifications ideas

I'm not sure about naming and where this option should be added

  • Rename state_alias to state_key ?
  • Move state_aliasfrom plan_options to run block ?

Target Release

1.11.x

Draft CHANGELOG entry

NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS

  • The terraform test command now supports a state_alias attribute in plan_option blocks during test runs. This feature allows multiple Terraform test runs to target the same infrastructure.

@kevineor kevineor requested a review from a team as a code owner December 10, 2024 11:05
Copy link
Member

@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.

Hi @kevineor, thanks for this. The implementation seems fine to me, just a couple of notes about naming and syntax. The solution here seems simple enough so I'm happy to work through this with you.

I'll just need to check in with our product manager as we have some test work lined up and I want to make sure this doesn't contradict or break anything we have planned, but I don't think there should be any problems.

Also, could you modify the CHANGELOG.md directly in this PR? Thanks!

@@ -839,6 +846,7 @@ var testRunOptionsBlockSchema = &hcl.BodySchema{
{Name: "refresh"},
{Name: "replace"},
{Name: "target"},
{Name: "state_alias"},
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be state_key or even just state rather than state_alias. Alias implies it's another name for something rather than what it's doing which is just overriding the default state key.

Copy link
Author

Choose a reason for hiding this comment

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

Ok i'll wait for a definitive decision on your side and i'll apply the change

I agree that it may be confusing. I was also thinking about run_key as mentioning the state seems weird as is never appears with terraform test

@@ -839,6 +846,7 @@ var testRunOptionsBlockSchema = &hcl.BodySchema{
{Name: "refresh"},
{Name: "replace"},
{Name: "target"},
{Name: "state_alias"},
Copy link
Member

Choose a reason for hiding this comment

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

I think I wouldn't put this within the run options block. The run options are a 1:1 mapping to actual run options a user can specify on the command line to the plan command, and overriding the state is not something that can be done this way. I think putting it here confuses things.

I think we can actually just have it at the root level of the run block. It's metadata about the run block rather than something that changes the behaviour of the plan/apply operation itself.

@liamcervante
Copy link
Member

I've spoken to the product team, and we'd like to hold off on merging this PR for a little while. We're planning on investigating specific controls not just around managing state in memory, but also reading and writing state from the filesystem during tests, and more fine-grained integrations with destroy operations.

I think this PR can work in tandem with all of these, but I do want to make sure we don't introduce something that is a one way gate and can't be rolled back without breaking changes. We plan on doing our discovery in Q1 of next year, which means that in terms of releasing this (assuming all goes well with the discovery) there shouldn't be any delay - we'll still aim to merge it as part of the 1.11 release. But we'll include this proposal as part of the general set of discovery work we do, and when we're sure this is compatible with everything else we want to do I'll come back and provide an update.

Thanks for your understanding, we do appreciate the contributions but we want to make sure that we're not painting ourselves into a corner with anything. Thanks!

@kevineor
Copy link
Author

Okay!

Thanks so much for the quick review!

I'll go ahead and incorporate your suggestion in the meantime.
Feel free to let me know if you need any further modifications!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to specify a state alias in a run block
3 participants