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

command: New -compact-warnings option #23632

Merged
merged 1 commit into from
Dec 10, 2019
Merged

Conversation

apparentlymart
Copy link
Contributor

When warnings appear in isolation (not accompanied by an error) it's reasonable to want to defer resolving them for a while because they are not actually blocking immediate work.

However, our warning messages tend to be long by default in order to include all of the necessary context to understand the implications of the warning, and that can make them overwhelming when combined with other output.

As a compromise, this adds a new CLI option -compact-warnings which is supported for all the main operation commands and which uses a more compact format to print out warnings as long as they aren't also accompanied by errors. This is mainly intended for use in automation scenarios where work is being sent to production, as opposed local development scenarios while working on changes to a module, but it can be used in either context. The intent is to ensure that the warnings still stay visible (or else the warning mechanism would become useless) while minimizing their weight in the overall output.

This also includes a change to the default behavior to consolidate warnings further so that there's only one instance of each distinct summary in the output. Aside from that additional consolidation, the default behavior is unchanged so that the warning messages can be presented with contextual information initially.

Full warning messages are always shown if there's at least one error included in the diagnostic set too, because in that case the warning message could contain additional context to help understand the error.


Warnings:

- Quoted type constraints are deprecated
  on deprecated-interp.tf line 3 (and 2 more)
- Interpolation-only expressions are deprecated
  on deprecated-interp.tf line 18

To see the full warning notes, run Terraform without -compact-warnings.

When warnings appear in isolation (not accompanied by an error) it's
reasonable to want to defer resolving them for a while because they are
not actually blocking immediate work.

However, our warning messages tend to be long by default in order to
include all of the necessary context to understand the implications of
the warning, and that can make them overwhelming when combined with other
output.

As a compromise, this adds a new CLI option -compact-warnings which is
supported for all the main operation commands and which uses a more
compact format to print out warnings as long as they aren't also
accompanied by errors.

The default remains unchanged except that the threshold for consolidating
warning messages is reduced to one so that we'll now only show one of
each distinct warning summary.

Full warning messages are always shown if there's at least one error
included in the diagnostic set too, because in that case the warning
message could contain additional context to help understand the error.
@apparentlymart apparentlymart requested a review from a team December 10, 2019 19:35
@apparentlymart apparentlymart self-assigned this Dec 10, 2019
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Dec 10, 2019
// We show full warnings if there are also errors, because a warning
// can sometimes serve as good context for a subsequent error.
useCompact := true
for _, diag := range diags {
Copy link
Member

Choose a reason for hiding this comment

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

If we only want to upgrade the output on errors, couldn't this just be HasErrors()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is some future-proofing for a possible future scenario where there is a third diagnostic severity. My goal here is to explicitly check that the entire diagnostic set is warnings, as required by the contract of DiagnosticWarningsCompact, rather than to indirectly check it by assuming that error is the only non-warning severity.

(If this comes up again, perhaps we can add an OnlyWarnings accessor to Diagnostics to encapsulate this, but so far I don't recall us needing this check anywhere else.)

@apparentlymart apparentlymart merged commit c06675c into master Dec 10, 2019
@apparentlymart apparentlymart deleted the f-compact-warnings branch December 10, 2019 19:53
@nazarewk
Copy link

@apparentlymart can this be used by default with environment variable?

@apparentlymart
Copy link
Contributor Author

Hi @nazarewk,

All command line options can be specified as environment variables:

export TF_CLI_ARGS_plan="-compact-warnings"
export TF_CLI_ARGS_apply="-compact-warnings"

@ghost
Copy link

ghost commented Jan 10, 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 and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cli enhancement sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants