-
-
Notifications
You must be signed in to change notification settings - Fork 989
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
Change default logging level to Info #1541
Change default logging level to Info #1541
Conversation
Recent PR (gruntwork-io#1510) changed logging, and also introduced default logging level. While it seemed like a good idea, it also introduced a lot of confusion, because in logrus `Println()` is an alias to `Info()` loglevel - so lots of messages were lost. This change restores previous behavior in logging output. Related: gruntwork-io#1529 Related: gruntwork-io#1530 Related: gruntwork-io#1531 Related: gruntwork-io#1532
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.
Thanks! I think this will fix a lot of issues, but make the logging quite noisy again. Is there some easy way to get a list of all the places we log something, so I could make a call on which ones should be changed to debug, info, etc?
@brikis98 just |
Any chance you could do a grep and dump the list of log messages here? I can then quickly scan and see which should be changed to other log levels. I'm so booked right now, I don't even have time to figure out the right |
Here's what I got from
|
`Printf()` is an alias, so this does not change any functionality - just makes logging more explicit.
@yorinasub17 Thanks for taking care of this! @brikis98 I have pushed a change that changes all
|
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 both!
Thanks Yori! I believe @amnk changed the
Debugf
Debugf
Leave as-is
Leave as-is
Leave as-is
Leave as-is
Leave as-is |
remote/remote_state_gcs.go
Outdated
@@ -117,7 +117,7 @@ func gcsConfigValuesEqual(config map[string]interface{}, existingBackend *Terraf | |||
} | |||
|
|||
if existingBackend.Type != "gcs" { | |||
terragruntOptions.Logger.Printf("Backend type has changed from gcs to %s", existingBackend.Type) | |||
terragruntOptions.Logger.Infof("Backend type has changed from gcs to %s", existingBackend.Type) |
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.
I think this one still needs to be tweaked
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.
A few last tweaks
Reviewing the related bugs:
I'm not sure if this PR addressed this or not. @amnk Could you try to repro and see if this issue is fixed with the logging updates in this PR?
I believe we are now using
I'm not sure if this issue is the same as #1530, or if it's about the need to hide logging prompts when
This seems very close to #1529, where |
This also updates integration tests to catch this error.
29e833c
to
31558c5
Compare
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! I'll kick off tests now.
Tests passed! Merging and release now. Thanks @amnk! |
@amnk @brikis98 @yorinasub17 the logs are still very verbose after the recent changes with log-level info. With log-level warn the logs verbosity is perfect, but then the error messages get lost. Is it possible to get the verbosity of warn with the addition of the error messages? Thanks |
This patch was the first one in the series, because changing of logging level was a surprise for many people :) So yeah, I will be working on another PR(s) to make it actually less verbose while still printing relevant error messages. @dudicoco btw, can you share specific log messages you think are not needed with default output? |
Thanks @amnk. The following log lines appear hundreds of times within the output and I believe are not necessary with the normal log level:
In particular the |
Hm, I believe these are necessary when we are interactively prompt the user if they want to run their
Not sure what this is. @amnk Could you investigate? |
@brikis98 when running In addition to hiding these when Thanks |
If stack run finished without errors, `summarizePlanAllErrors()` receives empty buffer and outputs empty line. This change ensures that only non-empty outputs are getting logged. Related: gruntwork-io#1541
If stack run finished without errors, `summarizePlanAllErrors()` receives empty buffer and outputs empty line. This change ensures that only non-empty outputs are getting logged. Related: gruntwork-io#1541
If stack run finished without errors, `summarizePlanAllErrors()` receives empty buffer and outputs empty line. This change ensures that only non-empty outputs are getting logged. Related: #1541
https://github.com/gruntwork-io/terragrunt/releases/tag/v0.28.8 should improve |
@brikis98 fyi, 0.28.8 still doesn't have binaries posted to github releases... |
@lorengordon Should be available now. Sorry about that! |
* Fix dead link in multiple aws accounts docs (gruntwork-io#1563) * Fix dead link in multiple aws accounts docs The link to AWS docs is now 404. The corrected link seems to most closely resemble the intended target. Other options to consider: https://aws.amazon.com/organizations/getting-started/best-practices/ https://docs.aws.amazon.com/controltower/latest/userguide/aws-multi-account-landing-zone.html * Link to AWS best practices for multi account docs * Whitespace removal (gruntwork-io#1573) * Fix empty outputs (gruntwork-io#1568) If stack run finished without errors, `summarizePlanAllErrors()` receives empty buffer and outputs empty line. This change ensures that only non-empty outputs are getting logged. Related: gruntwork-io#1541 * doc: contributing: fix broken link to circleci (gruntwork-io#1580) * Bump AWS SDK to version v1.37.7 to support AWS SSO (gruntwork-io#1537) * Add TargetPrefix as config input to access bucket logging (gruntwork-io#1507) * adding target-prefix ro access bucket logging * Updating test & example ! Note that this needs the terratest PR (gruntwork-io/terratest#767) to be merged in to work & be tested. * Updating Terratest dependency * testing for target prefix * Updating docs * Renaming folder * Updating to Debugf * Adding default value * WIP - parsing for TFstatelogs * Updating logic & docs * Adding a new test for default TargetPrefix in remote backend config * Introduce validate-inputs, which can be used to check for variable alignment (gruntwork-io#1572) * Introduce terragrunt-input-info, which can be used to check for variable alignment * Apply suggestions from code review Co-authored-by: Zack Proser <[email protected]> * Tidy go modules * Renamed input-info to validate-inputs * Switch missing required vars to errors * Handle -var and -var-file args * Update cli/validate_inputs.go Co-authored-by: Yevgeniy Brikman <[email protected]> * Make sure to check for dynamically passed in CLI args * Fix build * Handle automatically loaded var files * Remove plan args check * Clarify difference between getTerraformInputNamesFromVarFiles and getTerraformInputNamesFromCLIArgs * Address PR nit to move example in docs Co-authored-by: Zack Proser <[email protected]> Co-authored-by: Yevgeniy Brikman <[email protected]> * Use go1.16 to build arm64 binaries (gruntwork-io#1585) * Bump creack/pty to 1.1.11 (gruntwork-io#1582) Co-authored-by: Andy Bohne <[email protected]> * Add ability to specify working directory of hooks (gruntwork-io#1588) * Add ability to specify working directory of hooks * Fix build * Support dynamodb_endpoint attribute of S3 backend (gruntwork-io#1586) * Clarify non-interactive will not include external dependencies (gruntwork-io#1593) * add getTerragruntSource helper function (gruntwork-io#1575) * add getTerragruntSource helper function * update docs * update docs and tests for get_terragrunt_source_cli_flag() function * add use cases for get_terragrunt_source_cli_flag * Recursively extract forcedgetters until there are none (gruntwork-io#1594) * Remove all usage of get-plugins=false which is removed in 0.15.0 (gruntwork-io#1618) * Fix validate-inputs to support null defaults (gruntwork-io#1613) * Clarify context of find_in_parent_folders (gruntwork-io#1623) Co-authored-by: Paul <[email protected]> Co-authored-by: Yoriyasu Yano <[email protected]> Co-authored-by: amnk <[email protected]> Co-authored-by: Marco Molteni <[email protected]> Co-authored-by: David Wooldridge <[email protected]> Co-authored-by: Ina Stoyanova <[email protected]> Co-authored-by: Zack Proser <[email protected]> Co-authored-by: Yevgeniy Brikman <[email protected]> Co-authored-by: Andy Bohne <[email protected]> Co-authored-by: Andy Bohne <[email protected]> Co-authored-by: Alexey Remizov <[email protected]> Co-authored-by: Syed Hussain <[email protected]> Co-authored-by: David Alger <[email protected]>
Recent PR (#1510) changed logging, and also introduced default logging
level. While it seemed like a good idea, it also introduced a lot of
confusion, because in logrus
Println()
is an alias toInfo()
loglevel - so lots of messages were lost.
This change restores previous behavior in logging output.
Related: #1529
Related: #1530
Related: #1531
Related: #1532