Avoid caching when API call fails#164
Conversation
Check the return code of the bash function call and only store the result in the cache if the call was successful. This avoids caching empty values.
WalkthroughThe changes introduce consistent error handling across multiple functions in various Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (14)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/info.sh (1)
33-38: Simplify and strengthen exit-code checking for API call.Rather than capturing the output then immediately inspecting
$?, you can use theif ! …; thenpattern to both capture stdout and test the success in one step. It’s more concise and eliminates any ambiguity about which command’s exit code is being checked:-if info=$(bashio::api.supervisor GET /info false) -then - : -else - bashio::log.error "Failed to get info from Supervisor API" - return "${__BASHIO_EXIT_NOK}" -fi +if ! info=$(bashio::api.supervisor GET /info false); then + bashio::log.error "Failed to get info from Supervisor API" + return "${__BASHIO_EXIT_NOK}" +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/info.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/info.sh (1)
lib/log.sh (1)
bashio::log.error(190-195)
🔇 Additional comments (1)
lib/info.sh (1)
33-38: Correctly prevent caching when Supervisor API call fails.The added error handling ensures that the
"info"cache is only set on successful API responses, preventing invalid or empty data from being stored. This aligns perfectly with the PR’s goal to avoid caching failed API calls.
|
Without this, the first call prints an error like There are more such API calls, so If that error handling looks good, I'll add it to all API call sites which set the cache subsequently. |
Do you want to add them all to this PR? ../Frenck |
Yes, let me do it for all sites. |
agners
changed the title
agners
force-pushed
the
avoid-caching-if-api-call-fails
branch
from
4aa73b3 to
717ff46
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🛑 Comments failed to post (1)
lib/supervisor.sh (1)
185-203:
⚠️ Potential issueCritical: Fix variable assignment in
supervisor.country
The function mistakenly assigns the JSON payload tochannelinstead ofcountry, leading to no payload being sent for the country setter. This will break the POST call's body.Apply this diff to correct the variable name:
-function bashio::supervisor.country() { +function bashio::supervisor.country() { local country=${1:-} bashio::log.trace "${FUNCNAME[0]}:" "$@" if bashio::var.has_value "${country}"; then - channel=$(bashio::var.json country "${country}") - bashio::api.supervisor POST /supervisor/options "${country}" + country=$(bashio::var.json country "${country}") + bashio::api.supervisor POST /supervisor/options "${country}" bashio::cache.flush_all else bashio::supervisor 'supervisor.info.country' '.country' fi }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# ------------------------------------------------------------------------------ # Returns or sets the current country of the system. # # Arguments: # $1 Country to set (optional). # ------------------------------------------------------------------------------ function bashio::supervisor.country() { local country=${1:-} bashio::log.trace "${FUNCNAME[0]}:" "$@" if bashio::var.has_value "${country}"; then country=$(bashio::var.json country "${country}") bashio::api.supervisor POST /supervisor/options "${country}" bashio::cache.flush_all else bashio::supervisor 'supervisor.info.country' '.country' fi }
|
Coderabbitai references a change I was still working on and accidentally pushed. I dropped that again. @frenck done, PR should be complete now. |
frenck
added
the
bugfix
2 participants
Proposed Changes
Check the return code of the bash function call and only store the result in the cache if the call was successful. This avoids caching empty values.
Related Issues
Summary by CodeRabbit