Updated CopyTerraformFolderToTemp to include .terraform-version file #963
Conversation
The CopyTerraformFolderToTemp method will now copy the '.terraform-version' file if it exists in the source folder. This adds support for users who use the 'tfenv' tool to enforce that a certain version of Terraform is used in deployments and testing.
The .terraform-version file has been added to the 'full-copy' folder to resolve a failing unit test.
modules/files/files.go
Outdated
| pathParts := strings.Split(path, string(filepath.Separator)) | ||
| for _, pathPart := range pathParts { | ||
| if strings.Contains(pathPart, ".terraform-version") { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
NIT: If .terraform-version is a file, wouldn't it be the final part of the path? And if so, could this be simplified to filepath.Base(path) == ".terraform-version"?
modules/files/files.go
Outdated
| func CopyTerraformFolderToTemp(folderPath string, tempFolderPrefix string) (string, error) { | ||
| filter := func(path string) bool { | ||
| return !PathContainsHiddenFileOrFolder(path) && !PathContainsTerraformStateOrVars(path) | ||
| return (!PathContainsHiddenFileOrFolder(path) || PathIsTerraformVersionFile(path)) && !PathContainsTerraformStateOrVars(path) |
There was a problem hiding this comment.
NIT: this is getting hard to read / reason about with all the nots, ors, etc. Perhaps two lines will be easier to read?
if PathIsTerraformVersionFile(path) {
return true
} else if PathContainsHiddenFileOrFolder(path) || PathContainsTerraformStateOrVars(path) {
return false
} else {
return true
}There was a problem hiding this comment.
Fixed with a slight modification to match the guidelines.
|
Thanks for the review @brikis98 ! I made some corrections based on your comments and pushed the changes. The unit tests still succeed fine. |
|
Looks like pre-commit checks failed: Please run |
|
Whoops! Sorry about that, I re-ran it and pushed the branch again. Seems like my pre-commit install didn't work for some reason. |
|
Hm, some tests are failing, and continue to fail on re-run, so it's not clear this is some transient issue. I'll have to come back to this to look into it to see if it's related to this PR or not. |
|
Yeah I saw that, which is very odd. If I have time I may run the tests locally both on the trunk branch and mine as well. |
|
My apologies, I've been super buried, and completely lost track of this one. @infraredgirl Would you mind taking a look at the test failures here and seeing if they are relevant or if this is OK to merge? |
|
I re-ran the tests, and the results are as follows:
Given the above, I'll go ahead and merge this PR. Thanks @yardbirdsax for your contribution and your patience! |
The CopyTerraformFolderToTemp method will now copy the '.terraform-version' file if it exists in the source folder. This adds support for users who use the 'tfenv' tool to enforce that a certain version of Terraform is used in deployments and testing. This fixes #962.
The output of the unit tests is available here. Please note: I only ran the unit tests for the module containing the code I changed.
Thanks for your consideration!