-
Notifications
You must be signed in to change notification settings - Fork 113
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
Adds version output parsing #7
Conversation
+ provider.null v2.1.2 | ||
|
||
Your version of Terraform is out of date! The latest version | ||
is 0.12.26. You can update by downloading from https://www.terraform.io/downloads.html |
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 think the note could be suppressed by disabling checkpoint when running under test, which would make the tests less fragile.
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.
And perhaps we should even disable checkpoint for version
commands by default as the advice is ignored and it's not shown to the end-user anyway?
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.
Couple of comments and trivial merge conflict.
This PR can be a test of #12 once CI is set up - then it can land in v0.2.0.
Thanks!
tfexec/version.go
Outdated
// Version returns structured output from the `terraform version` command including both the Terraform CLI version | ||
// and any initialized provider versions. | ||
func (tf *Terraform) Version(ctx context.Context) (tfVersion *version.Version, providerVersions map[string]*version.Version, err error) { | ||
// TODO: support re-invocation of this? |
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 think Version()
should always invoke terraform version
- tfexec
is intended as a way to invoke terraform
CLI commands as transparently as possible.
If the consumer wants the cached versions, they can read the fields from the Terraform
struct.
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 agree with Katy here, I think that any caching responsibility should be up to the consumer, even if it's consumer within terraform-exec
itself. e.g. I assume at some point we may need to cache and/or transparently expose the version here
terraform-exec/tfinstall/tfinstall.go
Lines 133 to 136 in 22dfd35
err := runTerraformVersion(terraformPath) | |
if err != nil { | |
return "", fmt.Errorf("executable found at path %s is not terraform: %s", terraformPath, err) | |
} |
but any such caching/exposure decision should be made within that package, keeping tfexec
itself simple and transparent.
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.
Yeah I agree (about re-invocation at least, I dislike a field for this, will put a new commit together with thoughts).
Especially now that this is more info than just the tf version (provider versions), re-invocation is much more useful.
Rebased and pulled in your work in #16 |
@kmoe rebased and added runtime tests for 0.11, 0.12, and 0.13. |
The wintest failure is Go 1.12 it seems. |
Yes, the Windows executor is using Go 1.12. It seems it's not happy with the use of |
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.
Please rebase following #37. Once CI passes we can merge.
All 🟢! |
No description provided.