-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Use a single logger instance #1612
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
Conversation
By using a single logger instance we avoid adding log entries to the logs written by `Stack.summarizePlanAllErrors`.
Thanks for the PR! Could you provide a bit more context on what this does? What's the issue you're trying to fix? What does the log output look like now and what will it look like with this change? |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for submitting this pull request. |
WalkthroughThis update tweaks how logging is handled when cloning Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
options/options.go (1)
264-264
: Good change to reuse the logger instance 👍This is a smart improvement! Instead of creating a completely new logger each time we clone the options, we're now reusing the existing logger and just adding contextual information with the
WithField
method.This approach:
- Prevents duplicate log entries (the main goal of this PR)
- Maintains the logging context while adding new information
- Is more efficient than creating new logger instances
The logrus library is designed exactly for this use case - creating derived loggers with additional context.
You might consider using a more descriptive field name like "workingDir" instead of "prefix" to make it even clearer what this field represents in the logs.
-Logger: terragruntOptions.Logger.WithField("prefix", workingDir), +Logger: terragruntOptions.Logger.WithField("workingDir", workingDir),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
options/options.go
(1 hunks)util/logger.go
(0 hunks)
💤 Files with no reviewable changes (1)
- util/logger.go
This has gone stale. I'm sorry about that @boekkooi-fresh . We've done quite a bit of work on our logging, and we hope this is resolved. |
By using a single logger instance we avoid adding log entries to the logs written by
Stack.summarizePlanAllErrors
.Summary by CodeRabbit
No user-facing features or visible changes in functionality.