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

terraform test: Collect variables from default var file within testing directory #34341

Merged
merged 9 commits into from Dec 4, 2023

Conversation

JKIPIKA
Copy link
Contributor

@JKIPIKA JKIPIKA commented Dec 1, 2023

This is a duplicate of #34276 for the merge to the main branch

@liamcervante liamcervante self-requested a review December 1, 2023 15:08
@liamcervante liamcervante changed the title Add logic to collect variables for terrafrom test terraform test: Collect variables from default var file within testing directory Dec 1, 2023
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 this looks good now!

@JKIPIKA, do you want to take a look at the commit I made and make sure you're happy with it and understand everything? Since it'll be your name associated with the final merge, I wanted to give you a chance to raise any concerns.

Once you've confirmed you're happy with everything, I can approve and merge!

Thanks so much for your work on this!

Comment on lines +1225 to +1232
if path.Dir(file.Name) == runner.Suite.TestingDirectory {
// If the file is in the testing directory, then also include any
// variables that are defined within the default variable file also in
// the test directory.
for name, value := range runner.Suite.GlobalTestVariables {
runner.globalVariables[name] = value
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@JKIPIKA, I resolved the diffs by undoing your changes to the other function and adding the relevant change directly in here instead.

You didn't do anything wrong, this function isn't available within the v1.6 branch so you did the right thing in your original PR. It's just we will have introduced this function at some point after the v1.6 launch for managing the global variables and it's much easier to add here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I get it now, thanks for explanation!

@JKIPIKA
Copy link
Contributor Author

JKIPIKA commented Dec 2, 2023

@liamcervante, I checked your commit, I can see that you moved my logic into initVariables() function, as it has the file reference. In the v1.6 branch I had access to file in the GetVariables() function that I used for the logic. I'm happy with your changes, and I'm glad that could help with making Terraform better.
You can approve the merge :)

@liamcervante liamcervante merged commit c98e355 into hashicorp:main Dec 4, 2023
6 checks passed
Copy link

github-actions bot commented Dec 4, 2023

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link

github-actions bot commented Jan 4, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants