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

nixos/samba: Requires shares to be mounted #99065

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

Conversation

@aneeshusa
Copy link
Contributor

@aneeshusa aneeshusa commented Sep 29, 2020

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.
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

@aneeshusa aneeshusa commented Sep 29, 2020

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

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 29, 2020

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

@aneeshusa aneeshusa commented Oct 31, 2020

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.

Copy link
Contributor

@doronbehar doronbehar left a comment

Tested on my system update to work.

@doronbehar doronbehar dismissed their stale review Oct 31, 2020

I noticed something to be missing.

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 31, 2020

However, I take back my approval. Since:

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

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

@stale
Copy link

@stale stale bot commented Jun 4, 2021

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

@stale stale bot added the 2.status: stale label Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants