refactor: Use new logrus.Logger instead of default #7193
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use a new logrus.Logger instance instead of the default logrus singleton. Relying on the default singleton is problematic when dependencies (or transitive dependencies) also use logrus. When code paths in these dependencies modify the default singleton, it also impacts Skaffold logging. I ran into this issue while trying to upgrade the `ko` dependency to v0.10, and there was unwanted error-level log output messages from `amazon-ecr-credential-helper`. Context: - ko-build/ko#586 - awslabs/amazon-ecr-credential-helper#309 This change also moves us closer to not leaking the logger implementation dependency outside the `output/log` package. Tracking: GoogleContainerTools#7131
halvards
commented
Mar 15, 2022
| } | ||
|
|
||
| // New returns a new logrus.Logger. | ||
| // We use a new instance instead of the default logrus singleton to avoid clashes with dependencies that also use logrus. | ||
| func New() *logrus.Logger { | ||
| return logrus.New() | ||
| } | ||
|
|
||
| // KanikoLogLevel makes sure kaniko logs at least at Info level and at most Debug level (trace doesn't work with Kaniko) | ||
| func KanikoLogLevel() logrus.Level { |
There was a problem hiding this comment.
This function doesn't appear to be called anywhere, except for the unit test. I left the function signature unchanged, in case I missed something.
There was a problem hiding this comment.
Yeah, we've actually been using the global logrus logger, but we should definitely completely move off of that. I can make a follow up to this to have us use a local one that we own
Codecov Report
@@ Coverage Diff @@
## main #7193 +/- ##
==========================================
- Coverage 70.48% 68.48% -2.00%
==========================================
Files 515 560 +45
Lines 23150 26485 +3335
==========================================
+ Hits 16317 18139 +1822
- Misses 5776 7095 +1319
- Partials 1057 1251 +194
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
briandealwis
requested review from
MarlonGamez
and removed request for
briandealwis
|
Adding @MarlonGamez as he's our logging expert |
MarlonGamez
approved these changes
Mar 15, 2022
There was a problem hiding this comment.
thanks for making this change @halvards . As mentioned I can make a follow up to this to make further improvements
halvards
added a commit
to halvards/skaffold
that referenced
this pull request
Mar 23, 2022
The new `ko` build option to produce SBOM is disabled until we have a design for how this should work across Skaffold builders. Additional transitive dependencies (mainly authentication libraries for various image registries) means that the Skaffold binary grows by about 4.8MB. Related: GoogleContainerTools#7193 Tracking: GoogleContainerTools#7131
halvards
added a commit
to halvards/skaffold
that referenced
this pull request
Mar 24, 2022
The new `ko` build option to produce SBOM is disabled until we have a design for how this should work across Skaffold builders. Additional transitive dependencies (mainly authentication libraries for various image registries) means that the Skaffold binary grows by about 4.8MB. The change to labels_test.go works around the tests failing due to this change in `client-go` between `v0.21.3` and `v0.21.4`: kubernetes/client-go@c6c0ca0 That commit came from this k/k PR: kubernetes/kubernetes#102928 Related: GoogleContainerTools#7193 Tracking: GoogleContainerTools#7131
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use a new
logrus.Loggerinstance instead of the default logrus singleton.Relying on the default singleton is problematic when dependencies (or transitive dependencies) also use logrus. When code paths in these dependencies modify the default singleton, it also impacts Skaffold logging.
I ran into this issue while trying to upgrade the
kodependency to v0.10, and there was unwanted error-level log output messages fromamazon-ecr-credential-helper.Context:
This change also moves us closer to not leaking the logger implementation dependency outside the
output/logpackage.Tracking: #7131