Reduce init-time log verbosity in operator API types #7132
Conversation
|
Welcome @arnavgogia20! It looks like this is your first PR to karmada-io/karmada 🎉 |
Summary of ChangesHello @arnavgogia20, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where importing Karmada operator API types would unconditionally emit an Info-level log message during package initialization. This behavior is undesirable for a library package as it creates unsolicited noise for consumers. The change modifies the log statement to use a verbose log level ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
karmada-bot
added
the
size/XS
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where logging during package initialization was causing undesirable side effects for consumers. By changing the log level from Info to V(4), the log message will now only appear when verbose logging is explicitly enabled, which is the correct behavior for a library package. I've added one suggestion to make the code comment more descriptive, aligning it better with the repository's style guide. Overall, this is a valuable improvement.
| } | ||
| DefaultKarmadaImageVersion = releaseVer.ReleaseVersion() | ||
| klog.Infof("default Karmada Image Version: %s", DefaultKarmadaImageVersion) | ||
| // Use V(4) to avoid pollution during init |
There was a problem hiding this comment.
While the comment is helpful, it could be more descriptive to better align with the style guide's preference for comments that thoroughly explain the 'why'. A more detailed comment would better clarify why logging during init is being avoided.
| // Use V(4) to avoid pollution during init | |
| // Use a verbose log level to avoid side effects (unconditional logging) for consumers importing this package. |
References
- The style guide encourages comments that explain the intention of the code, rather than just stating the obvious. The provided examples show a preference for more descriptive comments explaining the rationale. (link)
There was a problem hiding this comment.
This comment seems reasonable.
There was a problem hiding this comment.
This comment seems reasonable.
Yes, i've done it, pls check....thankyouu
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7132 +/- ##
=======================================
Coverage 46.54% 46.55%
=======================================
Files 700 700
Lines 48134 48136 +2
=======================================
+ Hits 22403 22408 +5
+ Misses 24045 24043 -2
+ Partials 1686 1685 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hii @lonelyCZ @calvin0327, may you please consider reviewing this PR, thankyouu |
|
@arnavgogia20 Could you please use the template for PR description? In addition, please squash your commits after fixing the comments. |
The init function logs the Karmada image version at Info level, which causes noise during import. This change reduces the log level to V(4) and adds a descriptive comment to explain the decision, preventing side effects for downstream consumers. Signed-off-by: arnavgogia20 <arnavgogia404@gmail.com>
arnavgogia20
force-pushed
the
fix/issue-6957-log-noise
branch
from
4985ef7 to
d910185
Compare
@RainbowMango Yes, i'm done with both these fixes aswell, kindly check....thankyouu |
Relaxes a strict monotonic time check in TestGracefulEvictionRateLimiter_ExponentialBackoff that causes failures in CI environments with high CPU contention. Signed-off-by: arnavgogia20 <arnavgogia404@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
karmada-bot
added
size/S
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR reduces the log verbosity of the
init()function inoperator/pkg/apis/operator/v1alpha1/defaults.gofromInfotoV(4). Previously, importing this package would unconditionally print the "default Karmada Image Version" log, causing unwanted noise for downstream consumers. A comment has been added to explain this decision.Which issue(s) this PR fixes:
Fixes #6957
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?:
NONE