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

Copy validated Terraform to /tmp to avoid conflict #935

Merged
merged 3 commits into from
Jun 19, 2021

Conversation

zackproser
Copy link
Contributor

@zackproser zackproser commented Jun 18, 2021

  • Some tests do not copy their Terraform modules to /tmp first, which can lead to conflicts when ValidateAllTerraformModules is called
  • Avoid conflicts by always copying Terraform modules to be validated to /tmp
  • Fix an issue causing FindTerraformModulePathsInRootE to return .terraform hidden directories, leading to duplicate work and possible false positives.

@zackproser zackproser requested a review from yorinasub17 June 18, 2021 15:31
@zackproser zackproser requested a review from brikis98 as a code owner June 18, 2021 15:31
tfOpts := &Options{TerraformDir: dir}

//Copy the module to a tmp directory to avoid conflicts with tests that don't copy to /tmp
testFolder, err := files.CopyTerraformFolderToTemp(opts.RootDir, filepath.Base(dir))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that, due to an import cycle error, I had to use the files package's CopyTerraformFolderToTemp method. test_structure imports the terraform package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this logic correct? I think this is wrong for cases where there is a subdir (e.g., modules/agents/cloudwatch-agent).

I think you need to use filepath.Rel 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.

Thanks! Revised as discussed.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM assuming the test build passes (you can ignore the failing gcp_test). I had two nits, but they can be addressed in a subsequent PR.

Make sure to cut a backward incompatible release when you release this!

// Excludes any folders specified in the ValidationOptions.ExcludeDirs. IncludeDirs will take precedence over ExcludeDirs
// Use the NewValidationOptions method to pass relative paths for either of these options to have the full paths built
// Note that go_test is an alias to Golang's native testing package created to avoid naming conflicts with Terratest's
// own testing package. We are using the native testing.T here because Terratest's testing.T struct does not implement Run
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Mention that the this is here instead of terraform module to avoid import cycling.

@@ -1,4 +1,4 @@
package terraform
package test_structure
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Mention in the function docs that this is here instead of terraform module to avoid import cycling.

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: This file should probably still be called validate_struct.go, given the contents.

@zackproser
Copy link
Contributor Author

Thanks for the review! Going to merge this in now but will address your nits in a follow-up PR.

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

Successfully merging this pull request may close these issues.

2 participants