Fix various issues with bashio::addons()#175
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdjusts bashio::addons() parameter defaults and caching/filtering flow: cache_key now defaults to false, filter defaults to empty and is derived when omitted; cache reads/writes occur only when cache_key is provided; jq filtering runs only when a filter is present. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Addons as bashio::addons
participant Cache
participant API
participant JQ
Caller->>Addons: addons(slug?, cache_key?, filter?)
Addons->>Addons: normalize args (cache_key=false, filter derived if empty)
alt cache_key provided
Addons->>Cache: lookup(cache_key)
alt cache hit
Cache-->>Addons: cached JSON
else cache miss
Addons->>API: GET /addons or /addons/{slug}/info
API-->>Addons: JSON
alt filter non-empty
Addons->>JQ: apply filter -> filtered JSON
JQ-->>Addons: filtered JSON
Addons->>Cache: write(cache_key, filtered JSON)
else
Note right of Addons: Do NOT write overall response to cache
end
end
else no cache_key
Addons->>API: GET /addons or /addons/{slug}/info
API-->>Addons: JSON
alt filter non-empty
Addons->>JQ: apply filter -> filtered JSON
JQ-->>Addons: filtered JSON
end
end
Addons-->>Caller: return JSON (filtered or raw)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/addons.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/addons.sh (5)
lib/log.sh (2)
bashio::log.trace(137-140)bashio::log.error(198-203)lib/var.sh (2)
bashio::var.is_empty(84-94)bashio::var.has_value(66-76)lib/cache.sh (3)
bashio::cache.exists(16-26)bashio::cache.get(34-45)bashio::cache.set(54-71)lib/api.sh (1)
bashio::api.supervisor(20-108)lib/jq.sh (1)
bashio::jq(17-28)
🔇 Additional comments (2)
lib/addons.sh (2)
195-195: LGTM on output semantics.Printing with
printf '%s'avoids added newlines and preserves raw JSON when no filter is given.
206-221: No additional boolean-slug call sites detected. The only use offalse/trueas theslugargument is withinbashio::addons.installeditself, so slug normalization changes don’t affect other code.
PaulSD
force-pushed
the
addons
branch
2 times, most recently
from
7b29904 to
d85b8bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/addons.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/addons.sh (3)
lib/var.sh (1)
bashio::var.false(34-44)lib/cache.sh (2)
bashio::cache.exists(16-26)bashio::cache.set(54-71)lib/jq.sh (1)
bashio::jq(17-28)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/addons.sh (1)
174-196: Branch on normalized emptiness, not 'false', for API path and canonical cache.After slug normalization, use is_empty to choose list vs info endpoints/caches.
- if bashio::var.false "${slug}"; then + if bashio::var.is_empty "${slug}"; then if bashio::cache.exists "addons.list"; then info=$(bashio::cache.get 'addons.list') else info=$(bashio::api.supervisor GET "/addons" false) @@ - else + else if bashio::cache.exists "addons.${slug}.info"; then info=$(bashio::cache.get "addons.${slug}.info") else info=$(bashio::api.supervisor GET "/addons/${slug}/info" false)
🧹 Nitpick comments (2)
lib/addons.sh (2)
145-151: Docs misstate cache behavior; clarify filtered vs raw caching.The function always caches raw API payloads under canonical keys (addons.list or addons..info). The caller-provided cache_key is used only for filtered results when non-empty and not 'false'. Update wording accordingly.
-# $2 Cache key to store filtered results in (optional) -# (default/empty/'false' to cache only unfiltered results) +# $2 Cache key to store filtered results in (optional) +# (default/empty/'false' disables per-call filtered caching; +# raw payloads are always cached under canonical keys) -# $3 jq Filter to apply on the result (optional) -# (default/empty is '.addons[].slug' with no slug or '.slug' with slug) -# ('false' for no filtering) +# $3 jq filter to apply on the result (optional) +# (default/empty -> '.addons[].slug' when no slug; '.slug' when slug present; +# literal 'false' disables filtering)
174-196: Optional: derive and reuse a base_key for canonical raw caching to reduce duplication.Computing base_key once simplifies logic and reduces risk of mismatched keys.
+ # Canonical raw-cache key + local base_key + if bashio::var.is_empty "${slug}"; then + base_key="addons.list" + else + base_key="addons.${slug}.info" + fi @@ - if bashio::var.is_empty "${slug}"; then - if bashio::cache.exists "addons.list"; then - info=$(bashio::cache.get 'addons.list') + if bashio::cache.exists "${base_key}"; then + info=$(bashio::cache.get "${base_key}") + else + if bashio::var.is_empty "${slug}"; then + info=$(bashio::api.supervisor GET "/addons" false) ... - bashio::cache.set "addons.list" "${info}" - fi - else - if bashio::cache.exists "addons.${slug}.info"; then - info=$(bashio::cache.get "addons.${slug}.info") - else - info=$(bashio::api.supervisor GET "/addons/${slug}/info" false) - ... - bashio::cache.set "addons.${slug}.info" "${info}" - fi - fi + fi + bashio::cache.set "${base_key}" "${info}" + fiAlso applies to: 199-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/addons.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/addons.sh (4)
lib/var.sh (2)
bashio::var.is_empty(84-94)bashio::var.false(34-44)lib/log.sh (1)
bashio::log.trace(137-140)lib/cache.sh (2)
bashio::cache.exists(16-26)bashio::cache.set(54-71)lib/jq.sh (1)
bashio::jq(17-28)
🔇 Additional comments (2)
lib/addons.sh (2)
211-232: Sanity check: callers rely on 'false' sentinel; normalization above preserves this.The installed() wrapper passes false (Lines 222–227). With the normalization fix, both empty and 'false' work as “no slug” and default filter remains '.addons[].slug'. Looks good.
Please run a quick smoke test of these three flows:
- No slug, default filter: bashio::addons false
- Specific slug, default filter: bashio::addons 'self'
- No filtering: bashio::addons false false false
153-162: Empty slug is treated as a real slug; leads to GET /addons//info. Normalize slug first.When slug is empty (""), the code does not treat it as “no slug”. This causes single‑addon flow and a broken path. Normalize slug so both empty and literal 'false' mean “no slug”, and compute defaults from the normalized value.
-function bashio::addons() { - local slug=${1:-false} +function bashio::addons() { + local slug=${1:-false} + # Normalize: both empty and literal "false" mean "no slug" + if bashio::var.false "${slug}" || bashio::var.is_empty "${slug}"; then + slug="" + fi local cache_key=${2:-false} local filter=${3:-} - if bashio::var.is_empty "${filter}"; then - if bashio::var.false "${slug}"; then + if bashio::var.is_empty "${filter}"; then + if bashio::var.is_empty "${slug}"; then filter='.addons[].slug' else filter='.slug' fi fi
sairon
left a comment
sairon
left a comment
There was a problem hiding this comment.
Looking good, thank you!
2 participants
The default
cache_keyandfilterargument values inbashio::addons()caused problems when only a 'slug' was specified, or when an emptycache_keywas specified with a non-emptyfilter. See #163 for more details.This PR adds more robust default value handling for those arguments.
Fixes #163
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Documentation