Fix CLI availability check for CI environments#93
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the CLI availability check to rely solely on the process exit code sentinel (-1) instead of parsing version output or requiring a zero exit code, and adds logging for unavailable or failing CLI checks. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider avoiding the magic
-1exit code by extracting it into a named constant or enum inCLIResult/CLIExecutorso the sentinel behavior is easier to understand and reuse. - The
LOG.warnwhen availability fails would be more actionable if it included the exit code and not just the combined output or exception message, to help distinguish between different failure modes. - Catching a broad
ExceptioninisAvailable()may hide programming errors; if possible, narrow this to the specific process/IO-related exceptions that should map tofalsefor availability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding the magic `-1` exit code by extracting it into a named constant or enum in `CLIResult`/`CLIExecutor` so the sentinel behavior is easier to understand and reuse.
- The `LOG.warn` when availability fails would be more actionable if it included the exit code and not just the combined output or exception message, to help distinguish between different failure modes.
- Catching a broad `Exception` in `isAvailable()` may hide programming errors; if possible, narrow this to the specific process/IO-related exceptions that should map to `false` for availability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
For JAR-based CLIs, check file existence instead of spawning a --version subprocess. The subprocess is unreliable because Quarkus picocli returns exit code 1 for --version and JLine may redirect output to /dev/tty, leaving stdout/stderr empty.
b6b3a18 to
e7c37f6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CLIExecutor.isAvailable()returning false when the CLI is installed but--versionoutput goes to/dev/ttyinstead of stdout--plainflag is not appended for flags starting with-, so version output may not be captured in stdout/stderr--version, the old check (!output.isEmpty() || exitCode == 0) failedTest plan
HttpToolCliITCasetests pass in CISummary by Sourcery
Update CLI availability check logic to treat any non-sentinel exit code as indicating a present and executable CLI binary.
Bug Fixes:
Enhancements: