-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Switch from symlink and file copy to just file copy; upgrade dependencies; fix stack traces. #739
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
Conversation
The go-version library changed the behavior of how pre-release constraints are checked in hashicorp/go-version#35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left some comments, but all are minor nits that can be ignored.
cli/download_source.go
Outdated
// Filter out files in .terraform folders, since those are from modules downloaded via a call to terraform init, | ||
// and we don't want to re-download them. | ||
for _, terraformFile := range terraformFiles { | ||
if !strings.Contains(terraformFile, ".terraform") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this actually look at the path components? Is there a situation where you could have a hidden file that we want to actually delete, like .terraform-test.tf
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I noticed that and felt too lazy to do it right... But, fine, grumble grumble. 7c8a472
import ( | ||
"fmt" | ||
"github.com/gruntwork-io/terragrunt/util" | ||
"github.com/hashicorp/go-getter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I like to separate out stdlib imports from thirdparty imports from local imports. E.g:
import (
"fmt"
"net/url"
"os"
"github.com/hashicorp/go-getter"
"github.com/gruntwork-io/terragrunt/util"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://godoc.org/golang.org/x/tools/cmd/goimports can be used instead of go fmt
It works faster
terragrunt (go-getter-update*) » time goimports -w cli/file_copy_getter.go
goimports -w cli/file_copy_getter.go 0.01s user 0.00s system 82% cpu 0.015 total
terragrunt (go-getter-update*) » time go fmt cli/file_copy_getter.go
go fmt cli/file_copy_getter.go 0.42s user 0.41s system 244% cpu 0.341 total
and automatically manages imports (most of the time) and formats them
diff -u cli/file_copy_getter.go.orig cli/file_copy_getter.go
--- cli/file_copy_getter.go.orig 2019-06-12 08:20:02.000000000 +1200
+++ cli/file_copy_getter.go 2019-06-12 08:20:02.000000000 +1200
@@ -2,10 +2,11 @@
import (
"fmt"
- "github.com/gruntwork-io/terragrunt/util"
- "github.com/hashicorp/go-getter"
"net/url"
"os"
+
+ "github.com/gruntwork-io/terragrunt/util"
+ "github.com/hashicorp/go-getter"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I use IntelliJ and almost never look at the imports; all imports happen automatically while typing, so the order of them never felt important. That said, we do use goimports
with Terratest...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just ran goimports
and it updated 26 files. Probably not worth complicating this PR with that many changes. We should probably just do it in a separate PR and perhaps add it to the make fmt
command.
func TestCheckTerraformVersionMeetsConstraintGreaterDev(t *testing.T) { | ||
t.Parallel() | ||
testCheckTerraformVersionMeetsConstraint(t, "v0.9.4-dev", ">= v0.9.3", true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this was removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's in the commit message, but not obvious from the PR: the go-version
library we use under the hood was updated as part of this PR, and they changed how they handle version comparisons between "pre-releases" (e.g., v0.9.4-dev), so this comparison isn't actually valid anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it. Thanks for the explanation! I should have scanned the commit messages.
I've tested it and it actually fixes #736 👍 |
Fantastic, thanks for trying it out! |
Yori, thx for the review. Merging and releasing now. |
@@ -14,4 +14,6 @@ var RETRYABLE_ERRORS = []string{ | |||
"(?s).*Error installing provider.*TLS handshake timeout.*", | |||
"(?s).*Error configuring the backend.*TLS handshake timeout.*", | |||
"(?s).*Error installing provider.*tcp.*timeout.*", | |||
"(?s).*Error installing provider.*tcp.*timeout.*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated lines 16 and 17.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! #754
Fix copy/paste issue identified in #739 (review)
go-getter
uses symlinks for local files. This is fast and disk-space friendly, but it doesn't seem to work on windows. Moreover, you can't copy anything into a symlinked folder, as it would end up copying it back into the original, so this caused issues for Terragrunt. As a workaround, I was lettinggo-getter
create the symlink in one folder and then copying the files to another, but this was effectively the worst of both worlds—slow, not friendly to disk space, and complicated. In this PR, I've added by ownGetter
implementation that does file copying instead of using symlinks. I'm hoping this fixes failed to run mklink #733. It also allow us to copy existing symlinks without hitting infinite cycles, which I think fixes Terragrunt stopped copying symlinks from modules #736.go-getter
.MultiError
usage.