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

Adding Taint/Untaint command support #251

Merged
merged 5 commits into from
Dec 1, 2021
Merged

Conversation

rambabuiitk
Copy link
Contributor

Adding output name option support

Adding output name option support
@rambabuiitk
Copy link
Contributor Author

@radeksimko new PR

@radeksimko radeksimko added the enhancement New feature or request label Nov 22, 2021
@rambabuiitk
Copy link
Contributor Author

@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?

@rambabuiitk
Copy link
Contributor Author

@lafentres Can I get the review for this? I see the issue assigned to you.

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.

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?

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.

tfexec/taint.go Outdated Show resolved Hide resolved
tfexec/untaint.go Outdated Show resolved Hide resolved
tfexec/untaint.go Outdated Show resolved Hide resolved
tfexec/output.go Outdated Show resolved Hide resolved
@radeksimko
Copy link
Member

As for the failing Windows job I believe that's unrelated to your PR. I noticed we had a similar timeout-triggered failure on main recently: https://app.circleci.com/pipelines/github/hashicorp/terraform-exec/1334/workflows/d059b80e-43e4-4ea8-905f-165efaec939f/jobs/16081

I will look into raising the timeout in a separate PR.

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 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?

CHANGELOG.md Outdated Show resolved Hide resolved
tfexec/output.go Outdated Show resolved Hide resolved
tfexec/output.go Outdated Show resolved Hide resolved
tfexec/untaint.go Outdated Show resolved Hide resolved
tfexec/untaint.go Outdated Show resolved Hide resolved
tfexec/untaint.go Outdated Show resolved Hide resolved
tfexec/untaint.go Outdated Show resolved Hide resolved
tfexec/untaint_test.go Outdated Show resolved Hide resolved
tfexec/untaint_test.go Outdated Show resolved Hide resolved
tfexec/untaint_test.go Outdated Show resolved Hide resolved
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.

LGTM, thanks, I'll just apply that one last minor suggestion and get it merged.

tfexec/internal/e2etest/untaint_test.go Outdated Show resolved Hide resolved
@radeksimko radeksimko merged commit ea6fa6c into hashicorp:main Dec 1, 2021
@radeksimko radeksimko changed the title Adding Taint/UnTaint command support Adding Taint/Untaint command support Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting-response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants