support command chaining for --flash-cmd#153
Conversation
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
📝 WalkthroughWalkthroughThis pull request modifies the flash_image.sh script to wrap the FLASH_CMD execution with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/common/tasks/scripts/flash_image.sh (1)
81-92:⚠️ Potential issue | 🟠 MajorPreserve fail-fast semantics for chained flash commands.
Plain
sh -creturns the status of the last command in the snippet, so a value likecmd1; cmd2can make this task succeed even when the actual flash step failed earlier. If the contract is "allow chaining, but fail on the first unexpected error," use-ein both branches.Suggested change
OCI_USERNAME="${OCI_USERNAME}" \ OCI_PASSWORD="${OCI_PASSWORD}" \ - sh -c "${FLASH_CMD}" + sh -ec "${FLASH_CMD}" FLASH_EXIT=$? set -e # Restore errexit else # No credentials, run flash command directly # shellcheck disable=SC2086 set +e # Temporarily disable errexit to capture exit code - jmp shell ${JMP_SHELL_ARGS} -- sh -c "${FLASH_CMD}" + jmp shell ${JMP_SHELL_ARGS} -- sh -ec "${FLASH_CMD}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/flash_image.sh` around lines 81 - 92, The flash sequence can hide earlier failures because plain sh -c returns the last command's status; modify both jmp shell invocations that run "${FLASH_CMD}" so the shell runs with the -e (errexit) option (e.g. sh -e -c "${FLASH_CMD}" or prefix the command with "set -e; ${FLASH_CMD}") to make the chain fail fast, while still capturing the exit code into FLASH_EXIT and preserving the surrounding set +e / set -e behavior and use of JMP_SHELL_ARGS, OCI_USERNAME/OCI_PASSWORD and FLASH_CMD.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/common/tasks/scripts/flash_image.sh`:
- Around line 81-92: The flash sequence can hide earlier failures because plain
sh -c returns the last command's status; modify both jmp shell invocations that
run "${FLASH_CMD}" so the shell runs with the -e (errexit) option (e.g. sh -e -c
"${FLASH_CMD}" or prefix the command with "set -e; ${FLASH_CMD}") to make the
chain fail fast, while still capturing the exit code into FLASH_EXIT and
preserving the surrounding set +e / set -e behavior and use of JMP_SHELL_ARGS,
OCI_USERNAME/OCI_PASSWORD and FLASH_CMD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6434bea9-fe2d-4d73-ae59-a446ec22a358
📒 Files selected for processing (1)
internal/common/tasks/scripts/flash_image.sh
So users can do --flash-cmd "j power cycle && j storage flash ..."
Summary by CodeRabbit