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

cli: Better diagnostics for apply positional args #27679

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Feb 3, 2021

The previous changes removing support for using the trailing positional argument as a working directory missed a spot in the apply/destroy command implementation. We still support this argument for applying a saved plan:

terraform apply foo.tfplan

However, if you pass a positional path which doesn't "look like" a plan (for example, the path to a configuration directory), Terraform would silently ignore it and continue.

This commit fixes that by adding an error message if the user specifies a path which the plan loader rejects as not "looking like" a plan. This message includes a reference to the -chdir flag as a pointer about what to do next.

We also rearrange the error message when calling terraform destroy with a plan file argument, and add test coverage for the above. While we're here, update the destroy tests to copy the fixture directory, chdir, and defer cleanup.

@alisdair alisdair added the cli label Feb 3, 2021
@alisdair alisdair requested a review from a team February 3, 2021 19:21
@alisdair alisdair self-assigned this Feb 3, 2021
@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #27679 (d7613a0) into master (8057f19) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
command/apply.go 50.00% <100.00%> (+0.64%) ⬆️
internal/providercache/dir.go 67.34% <0.00%> (-6.13%) ⬇️

The previous changes removing support for using the trailing positional
argument as a working directory missed a spot in the apply/destroy
command implementation. We still support this argument for applying a
saved plan:

    terraform apply foo.tfplan

However, if you pass a positional path which doesn't "look like" a plan
(for example, the path to a configuration directory), Terraform would
silently ignore it and continue.

This commit fixes that by adding an error message if the user specifies
a path which the plan loader rejects as not "looking like" a plan. This
message includes a reference to the `-chdir` flag as a pointer about
what to do next.

We also rearrange the error message when calling `terraform destroy`
with a plan file argument, and add test coverage for the above. While
we're here, update the destroy tests to copy the fixture directory,
chdir, and defer cleanup.
@alisdair alisdair force-pushed the alisdair/apply-destroy-test-fixes branch from 0f5d110 to d7613a0 Compare February 4, 2021 15:27
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

👏 for error messages vs silence and strange behavior

@alisdair alisdair merged commit 32d2084 into master Feb 5, 2021
@alisdair alisdair deleted the alisdair/apply-destroy-test-fixes branch February 5, 2021 18:05
@ghost
Copy link

ghost commented Mar 8, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants