Improve handling of errexit option#192
Conversation
This reverts commit 69e4f1a.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnables Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TryLib as bashio::try
participant Subshell
participant State as __BASHIO_TRY_EXIT_STATUS
Caller->>TryLib: bashio::try "<command>"
TryLib->>Subshell: run command (errexit enabled inside)
Subshell-->>TryLib: exit status (0 / non‑0)
TryLib->>State: store exit status
TryLib-->>Caller: return (outer errexit preserved)
Caller->>TryLib: bashio::try.succeeded / bashio::try.failed
TryLib->>State: read stored exit status
TryLib-->>Caller: return OK / NOK based on stored status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/bashio`:
- Line 13: The direct call shopt -s inherit_errexit should be guarded for Bash <
4.4; update the code around the shopt invocation to first check BASH_VERSINFO
(major/minor) and only run shopt -s inherit_errexit when Bash version >= 4.4
(otherwise skip it) so the script won’t exit under set -e on older shells;
locate the existing shopt -s inherit_errexit line and replace it with a
version-check conditional that uses BASH_VERSINFO to decide whether to enable
inherit_errexit.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/jq.sh`:
- Around line 67-72: The conditional in lib/jq.sh has a duplicated `||` causing
a syntax error; fix the if statement that evaluates the result of bashio::jq and
bashio::var.has_value by removing the extra `||` so it reads as a single logical
OR between the two checks (i.e., ensure the condition is: if !
value=$(bashio::jq ... ) || ! bashio::var.has_value "${value}" then ...),
keeping the existing variables and calls (value, bashio::jq,
bashio::var.has_value, and returning __BASHIO_EXIT_NOK) intact.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
github-actions
bot
added
the
stale
|
not stale |
github-actions
bot
removed
the
stale
There was a problem hiding this comment.
Pull request overview
This PR improves Bashio’s error-handling semantics around errexit, especially for command substitutions and conditionals, by enabling inherit_errexit and introducing a bashio::try helper to run commands in an errexit-effective subshell while capturing the exit status.
Changes:
- Enable
inherit_errexitin the main entrypoints to makeerrexitpropagate into command substitutions. - Add
lib/try.shimplementingbashio::try,bashio::try.succeeded, andbashio::try.failed. - Make jq/config/service query construction and jq checks more robust in conditional contexts by avoiding fragile command substitutions and making
read -d ''non-fatal.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
lib/try.sh |
Adds a helper for running commands in an errexit-effective subshell and recording the exit status for later inspection. |
lib/bashio.sh |
Enables inherit_errexit and sources the new try module. |
lib/bashio |
Enables inherit_errexit in the wrapper entrypoint as well. |
lib/jq.sh |
Refactors jq helper predicates to capture jq output and treat jq failures as predicate failures. |
lib/config.sh |
Makes heredoc read -d '' non-fatal under errexit and refactors config.exists to avoid inline command substitution. |
lib/services.sh |
Makes heredoc read -d '' non-fatal under errexit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/try.sh (1)
32-37: Preserve callererrexitstate instead of forcing it on exit.At Line 36,
set -eis always re-enabled. If a caller intentionally haderrexitdisabled, this function mutates outer shell behavior after return. Prefer saving and restoring prior state.♻️ Proposed change
function bashio::try { + local __bashio_try_errexit_was_set=0 + [[ $- == *e* ]] && __bashio_try_errexit_was_set=1 set +e (set -e; "$@") __BASHIO_TRY_EXIT_STATUS=$? - set -e + if ((__bashio_try_errexit_was_set)); then + set -e + else + set +e + fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/try.sh` around lines 32 - 37, The function bashio::try currently unconditionally runs "set -e" on exit which mutates the caller's errexit state; fix by capturing the caller's errexit state at the start of bashio::try (e.g., test whether the "e" flag is present in $- or use set -o) into a local variable, run the subshell and set __BASHIO_TRY_EXIT_STATUS as before, and then restore the original errexit state (re-enable with "set -e" only if it was originally enabled, otherwise run "set +e") so the caller's shell options are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/try.sh`:
- Around line 32-37: The function bashio::try currently unconditionally runs
"set -e" on exit which mutates the caller's errexit state; fix by capturing the
caller's errexit state at the start of bashio::try (e.g., test whether the "e"
flag is present in $- or use set -o) into a local variable, run the subshell and
set __BASHIO_TRY_EXIT_STATUS as before, and then restore the original errexit
state (re-enable with "set -e" only if it was originally enabled, otherwise run
"set +e") so the caller's shell options are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30c67d87-e865-40bf-93ff-5698ad1390e8
📒 Files selected for processing (3)
lib/bashiolib/bashio.shlib/try.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/bashio
- lib/bashio.sh
Proposed Changes
The inherit_errexit option propagates and uses errexit in subshells in code like:
This variable assignment is the default all over the library. Though it won't help, if the function runs in a condition (like 'if'), because errexit is propagated then, but ignored. (So the fixes below in config.sh and jq.sh has no effect, just makes variable usage consistent.)
To make it possible to call subshells with effective errexit even in conditions, I made the bashio::try() function. This is useful if somebody needs to add a condition around an API call that changes some configuration, because all of those "setter" functions assume errexit is effective.
Some demonstrations
errexit works as expected (only 1 error message, no need for status checks):
The additional exit status check (in all "getter" function) saves the situation:
inherit_errexit makes errexit propagated and effective in variable assignment (default in the library):
Though inherit_errexit doesn't save the situation if there is a condition (the errexit is propageted, but ignored), we need the extra status checks (if somebody uses the library without storing the values in variables first):
And nothing saves the test functions like bashio::jq.exists()
How bashio::try helps
Related Issues
Summary by CodeRabbit
New Features
Refactor