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.

Sign up for GitHub

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/prometheus/alertmanager: use DynamicUser instead of nobody #59270

Merged
merged 1 commit into from Apr 10, 2019
Merged

nixos/prometheus/alertmanager: use DynamicUser instead of nobody #59270

merged 1 commit into from Apr 10, 2019

Conversation

basvandijk
Copy link
Member

Motivation for this change

See issue #55370.

@arianvp care to review?

The following which includes a test for alertmanager passes but I'll ask ofborg to confirm:

nix-build nixos/release.nix -A tests.prometheus.x86_64-linux

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@basvandijk
Copy link
Member Author

@GrahamcOfBorg test prometheus

arianvp
arianvp requested changes Apr 10, 2019
View reviewed changes
Copy link
Member

@arianvp arianvp left a comment

Choose a reason for hiding this comment

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

We should add an mkRemovedOptionModule entry to nixos/modules/rename.nix so people get warned if they used the user and group options.

Apart from that and my inline comments, lgtm

nixos/modules/services/monitoring/prometheus/alertmanager.nix
Restart = "always";
PrivateTmp = true;
DynamicUser = true;
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to keep PrivateTmp here I think

Copy link
Member Author

Choose a reason for hiding this comment

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

We can remove it because DynamicUser implies PrivateTmp.

@arianvp
Copy link
Member

arianvp commented Apr 10, 2019

Or we could just keep the user and group options. Though I am not sure if that adds anything in the case of DynamicUser. Pinging @flokli for opinion

@basvandijk
Copy link
Member Author

basvandijk commented Apr 10, 2019 •
edited

Or we could just keep the user and group options.

IIUC the User and Group systemd options become unnecessary when using DynamicUser.

@basvandijk
Copy link
Member Author

We should add an mkRemovedOptionModule entry to nixos/modules/rename.nix

Thanks, will do that.

@arianvp
Copy link
Member

arianvp commented Apr 10, 2019 •
edited

  • Could you add a comment (the string argument) in the removeOptions thing that tells why they were removed?
  • Could you add an entry to the change log?
    Apart from that lgtm

@basvandijk
Copy link
Member Author

Done. Thank you for the review. Will merge after checks are green.

@basvandijk basvandijk merged commit 38ae3fe into NixOS:master Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arianvp arianvp arianvp approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@basvandijk @arianvp @GrahamcOfBorg