-
Notifications
You must be signed in to change notification settings - Fork 123
chore: Removed dead code of rhsmcertd binary #3578
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideRemoved obsolete auto-registration interval handling in rhsmcertd, deprecated the --auto-registration-interval flag with warning messages, updated documentation and bash completion, and refactored the auto_register function signature and scheduling. Class diagram for Config struct and auto_register function after dead code removalclassDiagram
class Config {
int cert_interval_seconds
bool splay
bool auto_registration
}
class CertCheckData {
int interval_seconds
const char* next_update_file
}
class auto_register {
+void auto_register()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @jirihnidek - I've reviewed your changes - here's some feedback:
- Now that auto-registration interval is a no-op, remove the unused arg_reg_interval_minutes variable and its related branching in opt_parse_init_config for clarity.
- The N_() macro only marks strings for extraction and doesn’t perform runtime translation—use _() or gettext() in printf() if you want the message localized.
- You’re issuing the deprecation warning twice (warn() plus printf()); consider consolidating into a single logging call to avoid duplicate output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that auto-registration interval is a no-op, remove the unused arg_reg_interval_minutes variable and its related branching in opt_parse_init_config for clarity.
- The N_() macro only marks strings for extraction and doesn’t perform runtime translation—use _() or gettext() in printf() if you want the message localized.
- You’re issuing the deprecation warning twice (warn() plus printf()); consider consolidating into a single logging call to avoid duplicate output.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
No, it cannot be removed. It is still used in
No, when
The
|
|
FWIW... I can pr-verify that "Removed -r and --auto-registration-interval CLI option from bash completion script" is indeed true because existing testBashCompletion is failing on these two missing options when executed against branch jhnidek/rhsmcertd_deprecate_no_op_cli_option. See TestResultsReport for CLI: BashCompletion Tests I will update this test after this PR is merged to main. |
d7cc0d7 to
a0cd103
Compare
pkoprda
left a comment
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 from developer POV, let's wait for QE verification.
Also please rebase, the stylish CI check fail was fixed by #3582
* Card ID: CCT-1420 * When rhsmcertd binary was started with --auto-registration-interval CLI option, then the value of interval was ignored. The old logic of waiting was not used. The file /run/rhsm/next_auto_register_update was useless. * This commit does not change behavior (much). When the CLI option --auto-registration-interval is used, then it is still ignored, but we print warning messages that this CLI option is no-op and deprecated * Note: We also introduced another interval in: #3575 and changed manner of existing auto-registration interval. * Manual page of rhsmcertd is updated Signed-off-by: Jiri Hnidek <jhnidek@redhat.com>
a0cd103 to
d93266b
Compare
|
/packit build |
rhsmcertdbinary was started with--auto-registration-intervalCLI option, then the value of interval was ignored. The old logic of waiting was not used. The file/run/rhsm/next_auto_register_updatewas useless.--auto-registration-intervalis used, then it is still ignored, but we print warning messages that this CLI option is no-op and deprecatedSummary by Sourcery
Remove unused auto-registration interval logic from rhsmcertd, deprecate the --auto-registration-interval option with a warning, and update related documentation and completion scripts.
Enhancements:
Documentation:
Chores: