Skip to content

command: Warn instead of error for empty output#26036

Merged
alisdair merged 1 commit intomasterfrom
alisdair/output-empty-should-be-warning
Sep 1, 2020
Merged

command: Warn instead of error for empty output#26036
alisdair merged 1 commit intomasterfrom
alisdair/output-empty-should-be-warning

Conversation

@alisdair
Copy link
Copy Markdown
Contributor

When the output subcommand is called with no arguments, and there are no outputs to show, we previously rendered an error message but returned a non-error status code. This is confusing.

This commit changes the text UI to use a warning diagnostic, which makes it clearer that this is a non-error situation. We do not change the exit code or the text of the warning, so hopefully this is not considered a breaking change.

Fixes #25956

When the output subcommand is called with no arguments, and there are no
outputs to show, we previously rendered an error message but returned a
non-error status code. This is confusing.

This commit changes the text UI to use a warning diagnostic, which makes
it clearer that this is a non-error situation. We do not change the exit
code or the text of the warning, so hopefully this is not considered a
breaking change.
@alisdair alisdair requested a review from a team August 28, 2020 15:09
@alisdair alisdair self-assigned this Aug 28, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 28, 2020

Codecov Report

Merging #26036 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
command/output.go 47.41% <100.00%> (+1.00%) ⬆️
backend/remote/backend_common.go 55.25% <0.00%> (+0.67%) ⬆️
dag/marshal.go 54.44% <0.00%> (+1.11%) ⬆️

Copy link
Copy Markdown
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.

I agree that this is non-breaking, and reading some of the discussion on the linked issue, it was helpful in giving context about how returning an error without an error code here is confusing. I'd like someone else to +1 (probably Martin) this this is in the neighborhood of design-of-the-tool. But in general I like it?

@tobias-farrenkopf
Copy link
Copy Markdown

I'm very happy with this solution! 👍

Copy link
Copy Markdown
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

LGTM!

@alisdair alisdair merged commit 6d228cc into master Sep 1, 2020
@alisdair alisdair deleted the alisdair/output-empty-should-be-warning branch September 1, 2020 14:23
@ghost
Copy link
Copy Markdown

ghost commented Oct 13, 2020

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 Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Terraform output will always return successful

4 participants