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

Parse version -json output and fall back to plaintext #138

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

radeksimko
Copy link
Member

Closes #13

I didn't add more tests because it seemed like we'd be just testing mostly stdlib internals - either os.Exec or encoding/json. I'm open to ideas on what else to test though.

Also I think that exposing VersionOutput from the Version() method would be a good idea, but I avoided that just to retain compatibility, for now. Also it might be helpful to leverage (not yet published) library for parsing the provider addresses too. Either way I can open a separate issue for this.

@radeksimko radeksimko added the enhancement New feature or request label Mar 10, 2021
@radeksimko radeksimko requested a review from kmoe March 11, 2021 20:23
Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

This worked out better than I expected, as it looks like earlier Terraform versions don't error when -json is supplied to terraform version - they just ignore it.

Agree with testing approach here. We do need to add e2e tests for the providerVersions (see TODO in e2etest/version_test.go, but this can be done here or elsewhere. Let's create an issue if this isn't done now.

What would be the use case for exposing VersionOutput?

@radeksimko
Copy link
Member Author

What would be the use case for exposing VersionOutput?

I don't have a strong use case for it actually, I just thought maybe in some contexts it would be useful to know the platform, but I guess if it already differs from the host platform then it's unlikely we'd even be able to execute terraform anyway, so I guess there isn't a strong reason 🤷🏻

@kmoe
Copy link
Member

kmoe commented Mar 29, 2021

Great, let's keep a lookout for use cases until we're ready for 1.0, because now is the time to break things if we want to!

@radeksimko
Copy link
Member Author

Created #147

@radeksimko radeksimko merged commit e842b4c into main Mar 29, 2021
@radeksimko radeksimko deleted the f-version-json branch March 29, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse terraform version -json output
2 participants