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

Add ability to parse backend blocks present in a test file's run blocks, validate configuration #36541

Merged
merged 24 commits into from
Mar 6, 2025

Conversation

SarahFrench
Copy link
Member

@SarahFrench SarahFrench commented Feb 19, 2025

Description

Note: This PR is targeted at a feature branch instead of main, as I'd like to avoid creating a single, large PR for this work. My intention is to do a few PRs into that branch and eventually make a PR for the whole feature branch into main. That's when the change file would be included.

This PR is the first step of adding support for backend blocks to run blocks in terraform test:

  • implement parsing of backend blocks from run blocks.
  • implement validation of backend blocks in the context of runs.

Luckily we can reuse the existing code for parsing backend blocks, so validation is the main problem being addressed in this PR.

Validation requirements

  • Only state backends are supported
  • Only one backend block per run block
  • Only one backend block per internal state file (i.e. state_key value)
    • Update run blocks to have their state key value set earlier if not explicitly set via config; needed to allow proper validation here
  • For a given internal state file, the first command = apply run block must be where the backend block is used (if at all)
  • Backend blocks must be unique within a given test file

@SarahFrench SarahFrench added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Feb 19, 2025
@SarahFrench SarahFrench marked this pull request as ready for review February 19, 2025 20:20
@SarahFrench SarahFrench requested a review from a team as a code owner February 19, 2025 20:20
@SarahFrench SarahFrench requested a review from a team as a code owner February 21, 2025 12:22
@SarahFrench SarahFrench requested review from mikegolus and removed request for a team February 21, 2025 12:22
@SarahFrench SarahFrench changed the base branch from f-test-backend-block to f-controlling-destroys February 21, 2025 12:23
@SarahFrench SarahFrench removed the request for review from mikegolus February 21, 2025 12:23
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.

I think we're not checking anywhere that only the first run block for a given state can

Edit by Sarah : Need to check that the run with the the backend is the first apply command for that internal state

liamcervante
liamcervante previously approved these changes Mar 4, 2025
@SarahFrench
Copy link
Member Author

SarahFrench commented Mar 4, 2025

The crashes resulting from feb5a7b show that we're not setting the state key properly after the changes in 158151c.

I think that's because we don't set ConfigUnderTest until here:

diags = append(diags, buildTestModules(cfg, walker)...)

and that code runs after we parse individual run blocks etc.

@SarahFrench SarahFrench dismissed liamcervante’s stale review March 4, 2025 12:18

State keys aren't being set in time for validating use of backend blocks yet

@SarahFrench
Copy link
Member Author

The latest changes have:

  • Made sure that state keys for child modules are set during parsing of test files
  • Removed use of GetStateKey
  • Added some more tests that can adapt to runs being processed in different orders

@SarahFrench
Copy link
Member Author

Sorry for the back and forth here! Previously I thought the different ordering that impacted the acceptance test were due to new behaviour in the test graph, but it was just due to iterating over a map 🤦🏻 . I've addressed that in 75dfe43

Copy link
Member

@dsa0x dsa0x left a comment

Choose a reason for hiding this comment

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

Thanks for addressing that last bit 🫡

@SarahFrench
Copy link
Member Author

Thanks both! 🙇🏻

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