fix!: Do not enforce bootloader timeout default

[spetrosi]

Enhancement: Do not enforce bootloader timeout default

Reason: In the scenario where users manually change the timeout to be different than the default 5, the role force overwrites this setting to the default value of the bootloader_timeout variable.

Result: This fix sets the bootloader_timeout to null by default, so that the role changes the timeout only when users specify value for the bootloader_timeout variable in their playbook.

Issue Tracker Tickets (Jira or BZ if any): https://redhat.atlassian.net/browse/RHEL-211556

Summary by CodeRabbit

• Bug Fixes
  • Changed the default bootloader timeout behavior to be unset, so GRUB timeout values are not modified unless configured.
  • GRUB timeout updates now apply only when a timeout is explicitly set (covers both GRUB timeout representations).
  • Improved bootloader and user GRUB configuration ownership and permissions for safer operation.
• Documentation
  • Updated bootloader_timeout docs to reflect the new default (null) and clarify that the role only changes GRUB timeout when you set it.

[coderabbitai]

Warning

Review limit reached

@spetrosi, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e777afa6-0fd3-4091-8030-405a7e4cfe31

📥 Commits

Reviewing files that changed from the base of the PR and between a3eda6c and c6f0307.

📒 Files selected for processing (3)

• README.md
• defaults/main.yml
• tasks/main.yml

📝 Walkthrough

Walkthrough

Changes

Bootloader timeout handling

Layer / File(s)	Summary
Conditional timeout configuration defaults/main.yml, tasks/main.yml, README.md	The default bootloader_timeout is now null; GRUB timeout replacements run only when it is set, and documentation describes the unchanged native timeout behavior.
Bootloader configuration permissions tasks/main.yml	GRUB and user configuration files are assigned root ownership, group, and the configured file mode.

Possibly related PRs

• linux-system-roles/bootloader#220: Updates ownership and mode handling for the same GRUB and user configuration files.

Suggested reviewers: richm

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Format	⚠️ Warning	The PR description matches the template sections, but it omits the required Signed-off-by line.	Add a Signed-off-by: Your Name you@example.com line to the PR description (and Assisted-by if desired).

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows Conventional Commits format and accurately summarizes the change to stop enforcing the default bootloader timeout.
Description check	✅ Passed	The description includes all required template sections and clearly explains the change, reason, result, and ticket reference.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[spetrosi]

[citest]

[coderabbitai]

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@defaults/main.yml`:
- Line 3: Update the README documentation for bootloader_timeout to show null as
the default and state that existing GRUB timeout settings are preserved unless
the variable is explicitly set. Revise the example playbook text to clarify that
bootloader_timeout: 5 opts in to managing the GRUB timeout.

In `@tasks/main.yml`:
- Around line 106-108: Extend the tests in the tests/ directory to cover the
bootloader_timeout null/default path introduced by the “Update boot loader
timeout configuration” task. Verify that when bootloader_timeout is left null,
the task does not modify the existing bootloader timeout configuration.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 21ba376d-ea9a-4f12-8433-46f5f936ef12

📥 Commits

Reviewing files that changed from the base of the PR and between 9bc9805 and 9e627c8.

📒 Files selected for processing (2)

• defaults/main.yml
• tasks/main.yml

[richm]

If the grub timeout is already set, how does the user remove the timeout?

[richm]

If the grub timeout is already set, how does the user remove the timeout?

My suggestion would be - if bootloader_timeout is none then remove GRUB_TIMEOUT - if bootloader_timeout is not none then write GRUB_TIMEOUT={{ bootloader_timeout }}

[spetrosi]

If the grub timeout is already set, how does the user remove the timeout?

My understanding was that there is always some timeout. The default is 5, and you can probably disable it by setting to 0. I'll test what happens when it's not present in the config.

My suggestion would be - if bootloader_timeout is none then remove GRUB_TIMEOUT

Again same issue - users set the timeout to 30 manually, then run the role, and it removes their setting even though they didn't specify timeout variable. It forces users to go check what is their timeout and set bootloader_timeout: <their value> so that the role doesn't overwrite it and finishes in changed state.

[spetrosi]

Default timeout setting in /etc/default/grub:

GRUB_TIMEOUT=5

Default timeout setting in /boot/grub2/grub.cfg:

if [ x$feature_timeout_style = xy ] ; then
  set timeout_style=menu
  set timeout=5
# Fallback normal timeout code in case the timeout_style feature is
# unavailable.
else
  set timeout=5
fi

Removing the setting in /etc/default/grub allows reboot. Removing it from /boot/grub2/grub.cfg breaks the system and doesn't allow reboot. So this is a native setting that is not to be removed.

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@README.md`:
- Around line 103-107: Update the bootloader_timeout documentation to state that
a value of null preserves the existing timeout, matching the role’s skip
behavior, rather than referring to an unset variable. Add a playbook example
demonstrating both null preserving the current setting and an explicit integer
changing the timeout.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a75f3449-50b5-43f5-a658-7a163242cfb4

📥 Commits

Reviewing files that changed from the base of the PR and between 9e627c8 and e707f24.

📒 Files selected for processing (3)

• README.md
• defaults/main.yml
• tasks/main.yml

🚧 Files skipped from review as they are similar to previous changes (2)

• tasks/main.yml
• defaults/main.yml

[spetrosi]

[citest]

[spetrosi]

[citest]

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@tasks/main.yml`:
- Around line 106-108: Update the GRUB configuration template’s bootloader
timeout rendering to apply the same `bootloader_timeout is not none` guard used
by the “Update boot loader timeout configuration” task, so fresh installs omit
the timeout line when the value is null instead of rendering
`GRUB_TIMEOUT=None`.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 99d276b2-3ece-4e3d-bf9f-76f4e380e41b

📥 Commits

Reviewing files that changed from the base of the PR and between e707f24 and a3eda6c.

📒 Files selected for processing (3)

• README.md
• defaults/main.yml
• tasks/main.yml

🚧 Files skipped from review as they are similar to previous changes (1)

• defaults/main.yml

[spetrosi]

[citest]

[spetrosi]

[citest]