-
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
Adding Taint/Untaint command support #251
Conversation
Adding output name option support
@radeksimko new PR |
@radeksimko any chance I could get the review? the winbuildtest is failing, not sure why though...I couldn't find the reason from the logs...Is that mandatory? |
@lafentres Can I get the review for this? I see the issue assigned to you. |
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.
Hi @rambabuiitk
Thanks for your contribution!
Aside from my in-line comments I will say that we aim to also more gracefully handle version differences in tfexec and taint
in particular was introduced in v0.4.1
hashicorp/terraform@4ec31ec
untaint
was introduced in v0.6.13
hashicorp/terraform@c7f5450
Would you therefore mind adding a version check, similar to the one we have for fmt
here?
Lines 129 to 132 in 750bc90
err := tf.compatible(ctx, tf0_7_7, nil) | |
if err != nil { | |
return nil, fmt.Errorf("fmt was first introduced in Terraform 0.7.7: %w", err) | |
} |
Having checks for the individual flags would also be nice to have, but I won't block the PR on those.
As for the failing Windows job I believe that's unrelated to your PR. I noticed we had a similar timeout-triggered failure on I will look into raising the timeout in a separate PR. |
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.
Thank you for making these changes promptly.
I left you some more comments in-line.
Also - as suggested - do you mind adding each taint
and untaint
flag as an option?
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, thanks, I'll just apply that one last minor suggestion and get it merged.
Adding output name option support