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 supports for cmd: terraform add #209

Merged
merged 4 commits into from
Aug 12, 2021
Merged

Conversation

magodo
Copy link
Contributor

@magodo magodo commented Aug 12, 2021

Fix #208

Though the official release of the v1.1.0 is not shipped yet, while the alpha version is available. This means general users can use the terraform add feature already via the alpha CLI tool. As the tfinstall is able to install the alpha release, it means the tfexec users should also be able to use the alpha features (i.e. the add).

For the reason above, I've added a Latest_v1_1_alpha version for the test.

@radeksimko radeksimko added the enhancement New feature or request label Aug 12, 2021
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @magodo

I wouldn't say it's necessarily a rule that any tfinstall-installable version has to be supported in tfexec but as long as it's correctly version-gated then I think it's fine! 😄

The add command may still undergo some changes before getting out of alpha, but it's also equally likely that it will just land as is. With that in mind I also proposed a minor change in the comment, so that folks are more aware of potential BCs. That said they probably will be aware anyway as they have to opt into installing alpha version 🤷🏻‍♂️

The PR looks pretty good aside from my in-line comment about the out flag and the signature. Once that is addressed I'd be happy to merge it.

tfexec/internal/testutil/tfcache.go Outdated Show resolved Hide resolved
tfexec/add.go Outdated Show resolved Hide resolved
tfexec/add.go Outdated Show resolved Hide resolved
@magodo
Copy link
Contributor Author

magodo commented Aug 12, 2021

@radeksimko Thank you for the quick feedback! I've resolved the comments above, please take another look :)

@radeksimko radeksimko self-requested a review August 12, 2021 09:17
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Looking good, I'm happy to merge once that last nitpick is addressed 😃

tfexec/add.go Outdated Show resolved Hide resolved
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.

Add supports for terraform add command
2 participants