Skip to content

validate empty sentinel file path in NewFileRebootChecker - #1372

Merged
evrardjp merged 2 commits into
kubereboot:mainfrom
Ahmad-Faraj:fix/validate-empty-sentinel-path
Sep 30, 2026
Merged

evrardjp merged 2 commits into
kubereboot:mainfrom
Ahmad-Faraj:fix/validate-empty-sentinel-path

Conversation

@Ahmad-Faraj

Copy link
Copy Markdown
Contributor

NewFileRebootChecker already returns an error but never actually used it. if you pass an empty --reboot-sentinel the checker just stats "" forever and silently never reports a reboot, which is annoying to debug.

so this rejects an empty/blank path up front instead. dropped the old TODO and added a small test.

@Ahmad-Faraj Ahmad-Faraj reopened this Jun 22, 2026
Copilot AI review requested due to automatic review settings July 4, 2026 09:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request improves robustness of the file-based reboot sentinel checker by validating the sentinel file path early, preventing silent misconfiguration when an empty/blank --reboot-sentinel value is provided.

Changes:

  • Add upfront validation in NewFileRebootChecker to reject empty/blank file paths.
  • Remove the outdated TODO now that validation exists.
  • Add a unit test covering empty/blank input and a valid path case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pkg/checkers/checker.go Adds input validation to NewFileRebootChecker to prevent empty/blank sentinel paths.
pkg/checkers/checker_test.go Adds a unit test verifying error behavior for empty/blank paths and success for a valid path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/checkers/checker.go Outdated

@evrardjp evrardjp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's follow copilot suggestion

@Ahmad-Faraj

Copy link
Copy Markdown
Contributor Author

Done, trimming once and storing the trimmed path now.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

@Ahmad-Faraj

Copy link
Copy Markdown
Contributor Author

@evrardjp the trim change is in, 157fcaeb. Any chance of another look?

@evrardjp

evrardjp commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

I have to put that into the context of the other changes, but yeah I will work on it when I can. Thanks for your contribution.

@evrardjp

evrardjp commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

@Ahmad-Faraj we generally request for commits to be signed for your user (you can use git gpg/ssh signing, and adding those keys into your github user). This (kinda, as you could share private keys, but that's another topic) makes sure it's not someone else developing it for you (legal reasons).

If you can't I will find a solution, but this is not my favorite path.

Signed-off-by: Ahmad-Faraj <ahmedfrag4040@gmail.com>
Signed-off-by: Ahmad Faraj <ahmedfrag4040@gmail.com>
@Ahmad-Faraj
Ahmad-Faraj force-pushed the fix/validate-empty-sentinel-path branch from 5527c78 to a59adf3 Compare September 29, 2026 19:30
@Ahmad-Faraj

Copy link
Copy Markdown
Contributor Author

@evrardjp Signed now. Rebased on main with the same diff, both commits show Verified.

@evrardjp
evrardjp merged commit f2da2dc into kubereboot:main Sep 30, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants