Skip to content

Conversation

@dzhengfy
Copy link
Contributor

@dzhengfy dzhengfy commented Nov 22, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling to gracefully manage missing dependencies, ensuring the system properly defaults to package mode when the bootc command is unavailable instead of failing.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Dan Zheng <dzheng@redhat.com>
@coderabbitai
Copy link

coderabbitai bot commented Nov 22, 2025

Walkthrough

The is_image_mode function in virttest/utils_sys.py is modified to include an early guard that checks for the availability of the bootc command before proceeding. If which bootc fails, the function returns False immediately, bypassing any subsequent status checks via the session. This change alters the control flow and error handling to fail fast when the required bootc command is unavailable.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Single file and single function modification
  • Straightforward addition of an early guard/short-circuit check
  • Limited scope with clear conditional logic
  • Consider verifying:
    • Correctness of returning False as the intended behavior when bootc is unavailable
    • Behavior and side effects of the which bootc command execution
    • Whether existing tests cover this early-return scenario
    • Impact on downstream callers expecting different behavior when bootc is absent

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'utils_sys: check bootc available' clearly and concisely summarizes the main change: adding a check for bootc availability in the utils_sys module to control flow in is_image_mode function.

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@dzhengfy
Copy link
Contributor Author

dzhengfy commented Nov 22, 2025

Package mode:
No bootc installed:

utils_sys.is_image_mode()
False

Bootc installed:

utils_sys.is_image_mode()
False

Image mode:
Bootc is always installed:

utils_sys.is_image_mode()
True

@dzhengfy dzhengfy marked this pull request as ready for review November 22, 2025 03:31
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
virttest/utils_sys.py (1)

100-102: LGTM! Early guard correctly handles missing bootc command.

The implementation properly short-circuits when bootc is unavailable, avoiding unnecessary command execution and error messages. The logic aligns with the expected behavior described in the PR comments.

Optional: Consider adding debug logging for better observability.

Adding a log message when bootc is not found would improve debuggability and make the behavior more transparent:

 status, _ = cmd_status_output("which bootc", shell=True, session=session)
 if status:
+    LOG.debug("bootc command not found, assuming package mode")
     return False
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 22eae3a and 613e1bc.

📒 Files selected for processing (1)
  • virttest/utils_sys.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/utils_sys.py (2)
virttest/remote_commander/remote_runner.py (1)
  • shell (660-676)
virttest/utils_netperf.py (1)
  • session (122-124)
🪛 Ruff (0.14.5)
virttest/utils_sys.py

100-100: Function call with shell=True parameter identified, security issue

(S604)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

Copy link
Contributor

@smitterl smitterl left a comment

Choose a reason for hiding this comment

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

lgtm thank you

)
return result

status, _ = cmd_status_output("which bootc", shell=True, session=session)
Copy link
Contributor

Choose a reason for hiding this comment

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

avocado.utils.path provides find_command

Copy link
Contributor Author

@dzhengfy dzhengfy Dec 12, 2025

Choose a reason for hiding this comment

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

I know. Considering aautils repo, I guest avocado.utils.path will be moved. So now I am not sure if I should use it. What's your idea? @PaulYuuu

Copy link
Contributor

Choose a reason for hiding this comment

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

I am okay for both, the current implementation is simple and applicable to hosts/guest.

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.

4 participants