-
Notifications
You must be signed in to change notification settings - Fork 116
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 finding / installing Terraform by git ref #51
Conversation
This comment has been minimized.
This comment has been minimized.
tfinstall/download.go
Outdated
// TODO: actually use this header... | ||
// httpHeader := make(http.Header) | ||
// httpHeader.Set("User-Agent", "HashiCorp-tfinstall/"+Version) |
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.
@kmoe I noticed the header doesn't actually seem to be attached anywhere, but haven't gotten a chance to look at how to set it in go-getter yet. Just an FYI if you happen to poke around in here.
b2119a5
to
9f44e4c
Compare
@@ -4,10 +4,11 @@ go 1.14 | |||
|
|||
require ( | |||
github.com/davecgh/go-spew v1.1.1 | |||
github.com/go-git/go-git/v5 v5.1.0 |
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.
Why not use go-getter to get the git repo?
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.
I thought about it. go-getter uses CLI execution of git
, and a lot of code around SSH key handling and other things which I felt was overkill since the repo is hard coded and public. I guess this strategy is already requiring Go on your PATH, but trying to keep the requirements minimal.
These tests are a bit much for local development, but we do want to run them in CI (both PR and release workflows). An env var to switch all the |
Yeah, I will remove these from the defaults, but let you opt in to running master as "latest 0.13" or something with an env var. |
0a4e33d
to
39fbc23
Compare
testutil.Latest011, | ||
testutil.Latest012, | ||
testutil.Latest013, | ||
}, fixtureName, cb) | ||
} | ||
if override := os.Getenv("TFEXEC_E2ETEST_VERSIONS"); override != "" { |
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.
@kmoe I added this override, and I think we should maybe just run a cron CI run against master instead of on PR, but I'm not sure how to set it up in Circle, so will follow up in a future PR with that.
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.
added #70 for this
This partially solves #45, but we should also turn on cron based runs against it. |
206f7f9
to
565b764
Compare
This is a breaking change due to adding a
context.Context
totfinstall.Find
This really balloons up testing time, we should think through any other strategies to possibly speed that up?