Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

nixos/samba: Requires shares to be mounted #99065

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

nixos/samba: Requires shares to be mounted #99065

wants to merge 1 commit into from

Conversation

aneeshusa
Copy link
Contributor

Motivation for this change

This is useful in case the shares live e.g. on an external disk
or other location which is a noauto mount.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@aneeshusa
nixos/samba: Requires shares to be mounted
f3be4b5 
This is useful in case the shares live e.g. on an external disk
or other location which is a `noauto` mount.
@aneeshusa
Copy link
Contributor Author

I was able to successfully nix-build nixos/tests/samba.nix on a Linux machine running sandboxed Nix.

@doronbehar
Copy link
Contributor

Hmm seems to be making sense to me, though I'm wondering: Why don't we do it for every service that uses a certain directory to do something? I wouldn't expect the service to be taking care of that for me, in case I use non trivial fileSystems. For the record, you could use exactly this logic in your own configuration.nix and not rely on this patch.

@aneeshusa
Copy link
Contributor Author

I don't think it's something we'd want to put in every module, but I think it would be nice to have for services which are mostly meant for storing [large amounts of] data. For example, I copied this from the postgresql module which has had this for 8 years, since 2326c6d.

doronbehar
doronbehar previously approved these changes Oct 31, 2020
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on my system update to work.

@doronbehar doronbehar dismissed their stale review October 31, 2020 09:43

I noticed something to be missing.

@doronbehar
Copy link
Contributor

However, I take back my approval. Since:

nixpkgs/nixos/modules/services/network-filesystems/samba.nix

Line 59 in 8cab21b

unitConfig.RequiresMountsFor = "/var/lib/samba";

This is missing from the samba-smbd.service now.

@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@drupol
Copy link
Contributor

drupol commented Jun 6, 2023

Hello,

What's the status of this PR, I think it would be a good addition.

Do you please to refresh it so I can review it ?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 6, 2023
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doronbehar doronbehar doronbehar left review comments

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos 8.has: module (update) 9.needs: community feedback 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aneeshusa @doronbehar @drupol @wegank