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 support for providing a dest folder for tf copy #1054

Merged

Conversation

scott1138
Copy link
Contributor

Adds a new function similar to CopyTerraformFolderToTemp called CopyTerraformFolderToDest that allows providing a destination folder other than temp. This is useful when running on a build agent where a failure in the tests occurs and files are abandoned in the temp folder. With large provider downloads across multiple tests this can cause space issues.

Copy link
Contributor

@tonerdo tonerdo left a comment

Choose a reason for hiding this comment

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

I personally welcome this addition I've requested a few changes

modules/files/files.go Outdated Show resolved Hide resolved
modules/files/files.go Outdated Show resolved Hide resolved
modules/files/files.go Outdated Show resolved Hide resolved
@scott1138 scott1138 requested a review from tonerdo February 2, 2022 16:37
tonerdo
tonerdo previously requested changes Feb 2, 2022
Copy link
Contributor

@tonerdo tonerdo left a comment

Choose a reason for hiding this comment

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

Looks pretty good now. I have one final comment

@tonerdo tonerdo self-requested a review February 2, 2022 22:46
@tonerdo tonerdo dismissed their stale review February 2, 2022 22:51

Fixing the PR state

Copy link
Contributor

@tonerdo tonerdo left a comment

Choose a reason for hiding this comment

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

Looks pretty good now. I have one final comment

modules/files/files.go Outdated Show resolved Hide resolved
@scott1138 scott1138 requested a review from tonerdo February 3, 2022 03:53
modules/files/files.go Outdated Show resolved Hide resolved
@scott1138 scott1138 requested a review from yorinasub17 February 5, 2022 03:49
@scott1138
Copy link
Contributor Author

I can't see why the tests failed. I ran the Go tests locally and they passed. Any insights?
@yorinasub17 @tonerdo

@tonerdo
Copy link
Contributor

tonerdo commented Feb 9, 2022

@scott1138 Looks like there are some flaky tests. Looking into it

@scott1138
Copy link
Contributor Author

@tonerdo anything I can do to help?

@scott1138
Copy link
Contributor Author

@tonerdo Sorry to pester, just following up. The team that manages our agents doesn't like that I am writing data outside of my build directory, so I am hoping we can get this merged soon.

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! I took a look at the test failure and confirmed it's unrelated to these changes, so I am comfortable going through with merging and accepting this. Thanks for your patience!

@yorinasub17 yorinasub17 merged commit b0c746e into gruntwork-io:master Feb 18, 2022
@scott1138 scott1138 deleted the addDestFolderSupportForCopyTfFolder branch February 18, 2022 15:02
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.

3 participants