Samba: Add option to disable NetBIOS (nmbd) - #4705
Conversation
This script enables or disables the nmbd service based on the configuration setting.
Added netbios configuration for legacy SMB clients and reworded local_master description.
Reworded the 'local_master' option section and added details about the 'netbios' option, including its purpose and default value.
Updated sample configuration to include missing items and reordered some descriptions.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a configurable NetBIOS toggle to the Samba add-on, conditionally manages the ChangesSamba NetBIOS toggle
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant EnableCheck
participant Nmbd
participant SambaTemplate
Config->>EnableCheck: read netbios
EnableCheck->>Nmbd: create or remove service marker
Config->>SambaTemplate: provide netbios value
SambaTemplate->>Nmbd: allow or omit port 445 directive
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@samba/Dockerfile`:
- Around line 22-24: The S6 stage2 hook script referenced by the Dockerfile is
not executable, so startup will fail with a permission error. Update the Docker
image setup around the ENV for S6_STAGE2_HOOK and the enable-check.sh script so
that samba/rootfs/etc/s6-overlay/scripts/enable-check.sh is installed with
execute permissions (or explicitly chmodded) before the container starts, and
keep the hook path aligned with the script being run directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98a2dd9f-7cf2-4ad7-a165-076eea4b7548
📒 Files selected for processing (5)
samba/DOCS.mdsamba/Dockerfilesamba/config.yamlsamba/rootfs/etc/s6-overlay/scripts/enable-check.shsamba/translations/en.yaml
agners
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
I did not bump the revision number or add to the changelog but can do that if needed.
Yes, please also bump the version in config.yaml and add an entry to CHANGELOG.md.
…d fix line lengths in en.yaml
Co-authored-by: Stefan Agner <stefan@agner.ch>
|
Should all be good now. I would not consider changing the |
There is also wsdd implemented now. Shouldn't that make the Samba server discoverable for almost all Windows systems already? 🤔 |
For Windows I agree. But in some other cases (VLC on FireOS as a personal example) shares aren't discoverable without nmbd running or the service advertised with mDNS. |
Added config option and associated files to allow user to disable
nmbdservice startup. NetBIOS is not needed to access SMB shares with current operating systems. Added parameter toconfig.yamland implemented the enable/disable function by adding anenable-check.shscript and associatedDockerfilevariable.Description of netbios config parameter taken from #4468 since this text had already been reviewed. Made other updates to documentation to include a full sample config and update listing order.
I did not bump the revision number or add to the changelog but can do that if needed.
Summary by CodeRabbit