cmd_output(): Check if the command is executable before planning to run it

[bernhardkaindl]

cmd_output(): Check if the command is executable before planning to run it

The master branch is shared between XS 8.4 and XS 9:

• Recently, new cmd_output() calls were added for XS9 to run commands that don't exist on XS 8.4.
• Additionally, some tools supported by XS 8.4 are no longer supported in XS9

Both cases result in warnings or errors when attempting to execute these tools.

To avoid warnings about failing to execute commands at collection time, verify that the command is executable when compiling the list of commands to run, before executing all commands.

Last month, @MarkSymsCtx proposed this, and @rosslagerwall attempted to silence those warnings with more significant changes that would impact bugtool logging overall. That should be improved too, but it's a larger effort than just this targeted check to avoid those warnings specifically.

Most of the added lines are in the new comprehensive unit test in tests/unit/test_cmd_output.py, to test the functionality of all cases in the changed function. This ensures not only 100% code coverage but also that the function works exactly as expected, for both the existing code and the new executable check.

As shown below, GitHub Copilot PR review has no further review comments; all review comments it had were addressed in the in-depth pre-review, as shown in xenserver-next#30.

Pull Request Overview (generated using Copilot PR review)

This PR adds executable validation to the cmd_output() function to prevent warnings when attempting to run non-executable commands. The main change introduces a new is_deemed_executable() helper function that checks if commands are actually executable before adding them to the execution plan.

• Added is_deemed_executable() function to validate command executability for both absolute paths and PATH lookups
• Modified cmd_output() to skip non-executable commands early in the planning phase
• Updated test files and mock configurations to use absolute paths for better test coverage

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
xen-bugtool	Added is_deemed_executable() function and integrated executability check in cmd_output()
tests/unit/test_output.py	Updated test to use absolute path for cat command
tests/unit/test_load_plugins.py	Enhanced test documentation and updated to use absolute path
tests/unit/test_cmd_output.py	New comprehensive unit test file for cmd_output() function
tests/integration/dom0-template/etc/xensource/bugtool/mock/stuff.xml	Updated mock configuration to use absolute paths
.github/workflows/main.yml	Updated GitHub Actions workflow for coverage reporting

[Copilot]

Pull Request Overview

This PR adds executable validation to the cmd_output() function to prevent warnings when attempting to run non-executable commands. The main change introduces a new is_deemed_executable() helper function that checks if commands are actually executable before adding them to the execution plan.

• Added is_deemed_executable() function to validate command executability for both absolute paths and PATH lookups
• Modified cmd_output() to skip non-executable commands early in the planning phase
• Updated test files and mock configurations to use absolute paths for better test coverage

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
xen-bugtool	Added is_deemed_executable() function and integrated executability check in cmd_output()
tests/unit/test_output.py	Updated test to use absolute path for cat command
tests/unit/test_load_plugins.py	Enhanced test documentation and updated to use absolute path
tests/unit/test_cmd_output.py	New comprehensive unit test file for cmd_output() function
tests/integration/dom0-template/etc/xensource/bugtool/mock/stuff.xml	Updated mock configuration to use absolute paths
.github/workflows/main.yml	Updated GitHub Actions workflow for coverage reporting

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[GeraldEV]

LGTM, it makes sense to check if these commands are likely to work if we have a single repo which will run on multiple versions - but is there/should there be a way of making it clear which commands did/didn't run? Possibly beyond checking the resulting status report? Would it be useful to record why commands weren't run? Should most of these failures (e.g. poorly defined commands) be caught in testing before they can fail in a customer's environment?

[MarkSymsCtx]

LGTM, it makes sense to check if these commands are likely to work if we have a single repo which will run on multiple versions - but is there/should there be a way of making it clear which commands did/didn't run? Possibly beyond checking the resulting status report? Would it be useful to record why commands weren't run? Should most of these failures (e.g. poorly defined commands) be caught in testing before they can fail in a customer's environment?

For the concrete example that triggered this which is the migration from iptables to firewalld we would want to get support into the status-report ahead of the feature going live and still retain compatibility with the systems using iptables. It's not directly a version thing although it is expected that XS9 will have firewalld at public preview. There are of course possibilities for future things like this were we add new things to the base install and want the status-report to support from day1 so making it forward as well as backward compatible makes sense.

[bernhardkaindl]

@GeraldEV, I added logging of the skipped commands to the bugtool logfile for traceability.

[bernhardkaindl]

@GeraldEV your last review comment was:

LGTM, it makes sense to check if these commands are likely to work if we have a single repo which will run on multiple versions - but is there/should there be a way of making it clear which commands did/didn't run? Possibly beyond checking the resulting status report? Would it be useful to record why commands weren't run? Should most of these failures (e.g. poorly defined commands) be caught in testing before they can fail in a customer's environment?

Mark replied with: #141 (comment)

And I added logging of the skipped commands to the bugtool logfile for traceability since.

Could you re-review the PR now? (add your approval?)