fix: Fix incorrect changed reporting in bootloader_settings module#222
Conversation
|
● Here are three scenarios where the old code misbehaves and ours fixes it:
bootloader_settings: Old behavior: Iteration 1 adds mitigations=auto → apply_command sets changed=True. New behavior: Both iterations record actions in result["actions"]. After the loop,
bootloader_settings: Assume the DEFAULT kernel already has exactly args="quiet ro". Old behavior: rm_boot_args unconditionally generates grubby --remove-args='quiet New behavior: needs_replacement() compares sorted(["quiet","ro"]) against
bootloader_settings: Assume the rescue kernel has args="" (no args). Old behavior: rm_boot_args generates grubby --remove-args='' (a no-op command) → New behavior: rm_boot_args sees empty args → returns early, no no-op command |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe bootloader module now derives Bootloader settings behavior
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #222 +/- ##
==========================================
+ Coverage 78.43% 80.44% +2.00%
==========================================
Files 2 3 +1
Lines 255 363 +108
==========================================
+ Hits 200 292 +92
- Misses 55 71 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
library/bootloader_settings.py (1)
471-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard against divergence between
get_replaced_argsandmod_boot_args.
needs_replacementrelies onget_replaced_argsproducing exactly the arg setmod_boot_argswould leave after a full removal (duplicate-name dedup, absent filtering, etc.). These two are computed independently. Ifmod_boot_argsnormalization ever changes without a matching update here,needs_replacementcould returnFalsewhile a replacement is still needed, silently leaving stale args — the exact idempotency failure this PR targets. Consider a unit test asserting the desired token set fromget_replaced_argsmatches the post-replacement result ofmod_boot_argsfor the duplicate/absent cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/bootloader_settings.py` around lines 471 - 497, Add a unit test covering duplicate-name and absent options that compares the token set returned by get_replaced_args with the arguments produced by mod_boot_args after a full replacement/removal operation. Use this test to enforce that both paths apply identical deduplication and absent filtering, preserving needs_replacement idempotency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@library/bootloader_settings.py`:
- Around line 471-497: Add a unit test covering duplicate-name and absent
options that compares the token set returned by get_replaced_args with the
arguments produced by mod_boot_args after a full replacement/removal operation.
Use this test to enforce that both paths apply identical deduplication and
absent filtering, preserving needs_replacement idempotency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 91948e37-72b1-4e07-a5f5-425dacb93e1f
📒 Files selected for processing (2)
library/bootloader_settings.pytests/unit/test_bootloader_settings.py
| + "arg_with_int_value_absent=1 arg_without_val_absent' --copy-default" | ||
| ) | ||
| self.mock_module.run_command.assert_called_once_with(expected_cmd) | ||
| self.assertEqual(self.result["changed"], True) |
There was a problem hiding this comment.
change this to self.assertEqual(self.result["changed"], False) if nothing is changed?
4fe4693 to
134c160
Compare
|
[citest] |
|
lgtm - just fix the python black formatting then merge |
Fix the module reporting incorrect changed status in several cases: - apply_command() unconditionally set changed=True on every write, even when the system was already in the desired state - mod_boot_args() reset changed=False when no diff was found, overwriting a legitimate changed=True from a prior loop iteration - previous: replaced always reported changed=True by running rm_boot_args + mod_boot_args even when current args already matched the desired state - rm_boot_args generated a no-op grubby command when the kernel had no args to remove Now changed is computed once after the loop from the actions list. Add needs_replacement() to skip the remove+re-add cycle for previous: replaced when current args already match desired args. Update unit tests to match, add tests for new functions. Assisted-by: Cursor AI
134c160 to
0a59605
Compare
|
[citest] |
2 participants
Enhancement: Fix the module reporting incorrect changed status in several cases
Reason: Sometimes the role reported incorrect changed status.
Result: Now changed is computed once after the loop from the actions list. Add needs_replacement() to skip the remove+re-add cycle for previous: replaced when current args already match desired args.
Issue Tracker Tickets (Jira or BZ if any): https://redhat.atlassian.net/browse/RHEL-180862
Assisted-by: Cursor AI
Summary by CodeRabbit