-
Notifications
You must be signed in to change notification settings - Fork 860
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
Do not log detected resource #5546
Do not log detected resource #5546
Conversation
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.
+1 to this option (rather than #5545)
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.
Tests need to be updated to no longer expect the logged output.
Ah. I ran tests locally but only in the package I modified. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5546 +/- ##
==========================================
- Coverage 94.95% 94.95% -0.01%
==========================================
Files 308 308
Lines 7927 7921 -6
Branches 1604 1603 -1
==========================================
- Hits 7527 7521 -6
Misses 400 400
🚀 New features to boost your workflow:
|
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.
The main difference between the two approaches is the extra log available when using verbose logging, which we're not sure is used anyway. So this just simplifies the code. Is that right?
Yes it removes the "feature" of verbosely logging the resource after detection, but actually it's a duplicate log anyway |
This removes the logResources call which was triggering a warning that asynchronous resources were accessed before being resolved. Alternative to #5545
Originally posted by @trentm in #5545 (comment)