samba: migrate to app-based folder mappings and share names, bump base image - #4736
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Samba add-on to align with Home Assistant’s newer “app” terminology and Supervisor folder mappings, while preserving backward compatibility for existing SMB clients and stored configurations.
Changes:
- Rename share names from
addons/addon_configstolocal_apps/app_configs, keeping legacy share names available and pointing at the same paths. - Add startup migration to rewrite stored
enabled_sharesvalues to canonical (lowercase + renamed) values via the Supervisor options API. - Bump add-on version to
12.9.0and update the base image to3.24-2026.06.1.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| samba/rootfs/usr/share/tempio/smb.gtpl | Adds new share stanzas and maps legacy share names to the new mount paths for compatibility. |
| samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run | Extends startup migration to rename legacy enabled_shares values and deduplicate them. |
| samba/DOCS.md | Updates documentation to the new share names and explains backward compatibility behavior. |
| samba/config.yaml | Updates defaults/schema to accept new share names and migrates folder mappings. |
| samba/CHANGELOG.md | Documents the rename/migration behavior and base image update for 12.9.0. |
| samba/build.yaml | Updates base image tags to 3.24-2026.06.1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
56
to
58
| enabled_shares: | ||
| - "match(^(?i:(addons|addon_configs|backup|config|media|share|ssl))$)" | ||
| - "match(^(?i:(local_apps|app_configs|addons|addon_configs|backup|config|media|share|ssl))$)" | ||
| compatibility_mode: bool |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSamba 12.10.0 introduces ChangesSamba share migration and shutdown behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant init_smbd
participant OptionsAPI
participant smb_template
participant Samba
init_smbd->>OptionsAPI: Fetch persisted enabled_shares
init_smbd->>init_smbd: Canonicalize and deduplicate entries
init_smbd->>OptionsAPI: Post migration_payload
smb_template->>Samba: Emit canonical and legacy-compatible shares
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…e image (12.9.0) Rename the deprecated `addons` and `all_addon_configs` folder mappings to the new `local_apps` and `all_app_configs` names introduced in Home Assistant Supervisor (#6992), using the new default mount targets (/local_apps and /app_configs). Rename the exposed shares to `local_apps` and `app_configs` to match Home Assistant's app terminology. Existing installations are migrated automatically at start by extending the enabled_shares normalization added in 12.8.1: stored `addons`/`addon_configs` values are rewritten to `local_apps`/`app_configs` via the Supervisor options API. Because the options merge strategy overrides lists with the user's stored value, changing the default alone would not migrate existing installs, and the schema must keep accepting the legacy names so the app can start to run the migration (and so auto-update schema validation passes). Both the new and legacy share names stay exposed and point to the same directories, so existing SMB connections keep working while users move to the new names. Also update the base image to 3.24-2026.06.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
agners
force-pushed
the
samba-migrate-app-mapping
branch
from
July 18, 2026 17:37
2970f24 to
3021acb
Compare
agners
changed the title
mdegat01
requested changes
Jul 21, 2026
mdegat01
left a comment
mdegat01
left a comment
Member
There was a problem hiding this comment.
This makes sense and looks good. I asked claude how to inject some logging into the config of a samba share and it provided the snippet below. Think we should do something like that to let them know they have a client using a deprecated share and should switch it to the new one.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Address PR review feedback: - Log a warning via smbd's preexec whenever a client connects to a deprecated `addons`/`addon_configs` share, telling the user which new share to switch to (mdegat01). Use `logger -s` so the message reaches the add-on log (smbd stderr); the container has no syslog daemon reading /dev/log. - Update the `enabled_shares` UI translation to list the new canonical names (`local_apps`/`app_configs`) and note the legacy names are still accepted for backward compatibility (Copilot). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On a normal shutdown, s6-overlay brings the smbd/nmbd/wsdd longruns down with SIGTERM. The finish scripts recorded $((128 + 15)) = 143 as the container exit code, so every stop ended with 143 and the Supervisor warned that the add-on did not handle SIGTERM. Treat SIGTERM as the expected shutdown signal: still take the supervision tree down, but leave the container exit code at 0. Other signals and non-zero exits keep their previous failure-propagation behavior so genuine crashes still surface. Drop the now-unused exit_code_container read in the wsdd finish script. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/finish:24
- When smbd exits by signal for reasons other than SIGTERM, the finish script records the failure code but does not halt the supervision tree. This leaves the container running with smbd down (and may hide the failure from Supervisor). The tree should be halted for all signal exits; SIGTERM should remain a clean shutdown (exit 0) case.
if [[ "${exit_code_signal}" -ne 15 && "${exit_code_container}" -eq 0 ]]; then
echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode
fi
[[ "${exit_code_signal}" -eq 15 ]] && exec /run/s6/basedir/bin/halt
samba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/finish:24
- When nmbd exits by signal for reasons other than SIGTERM, the finish script records the failure code but does not halt the supervision tree. This leaves the container running with nmbd down (and may hide the failure from Supervisor). The tree should be halted for all signal exits; SIGTERM should remain a clean shutdown (exit 0) case.
if [[ "${exit_code_signal}" -ne 15 && "${exit_code_container}" -eq 0 ]]; then
echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode
fi
[[ "${exit_code_signal}" -eq 15 ]] && exec /run/s6/basedir/bin/halt
samba/translations/en.yaml:24
- Minor grammar/punctuation: use a period instead of a semicolon to avoid a sentence fragment in the description.
Only these values are allowed;
the configuration cannot be saved if any other value is in the list.
mdegat01
approved these changes
Jul 28, 2026
mdegat01
left a comment
mdegat01
left a comment
Member
There was a problem hiding this comment.
Looks good, I like the warning. Does the sigterm thing fix a known bug we can link in description/changelog? If so that'd be nice. If it was just something claude happened to discover during the other fix then guess its good as is.
Member
Author
It is something I noticed during testing (since failed state is much more prominent shown in frontend nowadays). |
nmbd and wsdd already exit 0 on SIGTERM: nmbd's handler calls exit(0), and only smbd registers an atexit killkids() that does kill(0, SIGTERM), which makes smbd terminate by the signal (exit 143). Their finish scripts' by-signal branch never fires in practice, so revert those two changes and keep the mask only in smbd/finish. Also link home-assistant/supervisor#6840 in the changelog. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
I've removed the changes from |
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.
Rename the deprecated
addonsandall_addon_configsfolder mappings to the newlocal_appsandall_app_configsnames introduced in Home Assistant Supervisor (#6992), using the new default mount targets (/local_apps and /app_configs).Rename the exposed shares to
local_appsandapp_configsto match Home Assistant's app terminology. Existing installations are migrated automatically at start by extending the enabled_shares normalization added in 12.8.1: storedaddons/addon_configsvalues are rewritten tolocal_apps/app_configsvia the Supervisor options API. Because the options merge strategy overrides lists with the user's stored value, changing the default alone would not migrate existing installs, and the schema must keep accepting the legacy names so the app can start to run the migration (and so auto-update schema validation passes).Both the new and legacy share names stay exposed and point to the same directories, so existing SMB connections keep working while users move to the new names.
Also update the base image to 3.24-2026.06.1.
Summary by CodeRabbit
local_appsandapp_configsSMB share names for local application content and configuration.addons/addon_configsshares available for backward compatibility.enabled_shares, including canonicalization, lowercasing, and deduplication.local_appsandapp_configs.