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 logic to collect variables for terrafrom test #34276

Closed
wants to merge 6 commits into from

Conversation

JKIPIKA
Copy link
Contributor

@JKIPIKA JKIPIKA commented Nov 20, 2023

I created the new function collectVariableValuesForTests that collects variables for the terraform test and the configuration to store and use collected values.

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 20, 2023

CLA assistant check
All committers have signed the CLA.

@JKIPIKA
Copy link
Contributor Author

JKIPIKA commented Nov 20, 2023

#34008

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 @JKIPIKA, this looks really good. Thanks for this.

I've added a couple comments in line, just tweaking the logic for this but overall the approach looks good.

It would also be good if you could add some tests for this. It doesn't need to be complicated:

  1. Add a new directory called tfvars_in_test_dir (or something) into here: https://github.com/hashicorp/terraform/tree/main/internal/command/testdata/test
  2. Create a simple configuration file that does something trivial like define some variables and echo them back as outputs.
  3. Create a nested tests directory.
  4. Create a variables file that should be loaded by default in the tests directory, and set values for the variables in the configuration.
  5. Create a sample test file that doesn't set any of the variables, but has a run block that validates the config outputs match the values set in the default variables file.
  6. Reference the tfvars_in_test_dir directory you created in step one from https://github.com/hashicorp/terraform/blob/main/internal/command/test_test.go#L33, and set the expected number of passed (1) and failed (0) tests and exit code (0).

You could repeat this to add tests to make sure any JSON variable files are loaded, or to make sure everything still works if you load an alternate test directory, if you wanted to really test things thoroughly.

Hopefully that all makes sense, let me know if you have any questions!

internal/command/meta_vars.go Outdated Show resolved Hide resolved
internal/command/meta_vars.go Outdated Show resolved Hide resolved
internal/backend/local/test.go Outdated Show resolved Hide resolved
internal/backend/local/test.go Show resolved Hide resolved
@liamcervante liamcervante self-assigned this Nov 21, 2023
@JKIPIKA
Copy link
Contributor Author

JKIPIKA commented Nov 21, 2023

Hi @liamcervante ! Thanks for the review, ill try to add the changes that you mentioned alongside with the tests.

@JKIPIKA
Copy link
Contributor Author

JKIPIKA commented Nov 28, 2023

Hi @liamcervante , I hope i understood all the things that you mentioned, can you please review the new changes ?

@crw
Copy link
Collaborator

crw commented Nov 28, 2023

@JKIPIKA could you please sign the CLA? See this comment: #34276 (comment) -- this is needed for us to eventually accept the changes. Thanks! I will let @liamcervante know you've made changes per his review.

@liamcervante liamcervante changed the base branch from v1.6 to main November 29, 2023 06:45
@liamcervante liamcervante requested a review from a team as a code owner November 29, 2023 06:45
@liamcervante liamcervante changed the base branch from main to v1.6 November 29, 2023 06:45
@liamcervante liamcervante removed the request for review from a team November 29, 2023 06:45
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.

Thanks @JKIPIKA, this looks very close.

Could you also rebase your changes against main? Hopefully, we'll release this for v1.7 which means we want to merge into main rather than the v1.6 branch.

And as Craig said you'll need to sign the CLA!

Thanks!

internal/backend/local/test.go Outdated Show resolved Hide resolved
internal/backend/local/test.go Outdated Show resolved Hide resolved
@@ -0,0 +1,18 @@
variables {
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 this file should be in the tests directory alongside the var files.

@JKIPIKA
Copy link
Contributor Author

JKIPIKA commented Nov 29, 2023

Hi @liamcervante, I finished last modifications that you mentioned and signed the CLA, but can you please provide me some hints how to effectively rebase it against main, as there are a lot of conflicts ?

I cherry-picked my changes into master branch, maybe if we can change the source branch on this PR to my main branch, would be this approach more simpler ?

@liamcervante
Copy link
Member

liamcervante commented Nov 30, 2023

@JKIPIKA, I don't think we can change the base branch for the pull request, and the diff is going to be complicated now we have a merge with the latest 1.6 branch in here.

I've looked at the diff for the changes you have in your main here. I think they look very close, so maybe the best thing to do is for you to close this PR, and create a new one that is merging your changes from main into our main. We can iterate from there to close out the final things - they mostly look like changes as a result of the cherry-picking, so we'll just have to undo some things.

JKIPIKA and others added 3 commits December 1, 2023 15:20
Co-authored-by: Liam Cervante <liam.cervante@hashicorp.com>
Co-authored-by: Liam Cervante <liam.cervante@hashicorp.com>
@JKIPIKA
Copy link
Contributor Author

JKIPIKA commented Dec 1, 2023

Hi, @liamcervante, I create the PR for the main branch #34341, but there are two changes which I think were done due to cherry-picking, I marked them with comment. Can it be somehow resolved ?

@liamcervante
Copy link
Member

Closed in favour of #34341

Copy link

github-actions bot commented Jan 1, 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 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants