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/icingaweb2: Replace most options with toINI #57716

Merged
merged 1 commit into from
Apr 4, 2019
Merged

nixos/icingaweb2: Replace most options with toINI #57716

merged 1 commit into from
Apr 4, 2019

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Mar 15, 2019

Motivation for this change

@infinisil asked me in the original PR (#55957) to reduce the number of options.
I hope this does it.

cc @ryantm

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 nox --run "nox-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.

@dasJ dasJ requested a review from infinisil as a code owner March 15, 2019 19:38
@dasJ
Copy link
Member Author

dasJ commented Mar 15, 2019

Oh, also asking for permission to backport to 19.03 as nobody is probably using the old options yet.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 15, 2019
@aanderse
Copy link
Member

Looking pretty awesome! I've become a fan of this approach after @infinisil explained the benefits to me as well, but I think database should still have nixos options so you can do passwordFile. I'd also argue for nixos options for the database because it's nice to abstract that away from the user. For this type of application database is not optional so it's nice to be able to provide the options for the user without requiring them to read any application specific documentation. Keeping users as close to services.appname.enable = true; and they have a working application should be the goal, not having to read application documentation unless they need something out of the ordinary.

What's your opinion about database options remaining nixos options?

@dasJ
Copy link
Member Author

dasJ commented Mar 16, 2019

@aanderse There is not really a way for passwordFile because sadly, ini files don't support includes.
Exposing the database stuff to the user sounds appealing, but I don't really see an upside. The key names are almost the same and the guidance the user gets by getting errors when i.e. using invalid field names is not it imo.

@aanderse
Copy link
Member

I see your point about passwordFile option. I suppose the other possibility would be to execute a script like this

DBPASS=$(head -n1 ${cfg.database.passwordFile})
and then store the output file in a secure directory. Definitely more work, but worth it?

@dasJ
Copy link
Member Author

dasJ commented Mar 24, 2019

@aanderse No - this doesn't work either. Trust me, if it was possible easily, I would have done it. Ini files neither have a way to include other files nor a way to execute arbitrary code. What you recommend is probably something like using sed to place the password into the configuration file - perhaps in the PreStart of the php-fpm pool. This requires a lot of code, makes the the entire module more unclear, and solves a problem that doesn't even exist when using the non-nix way (resources = null;) to specify the resources. I even mentioned the problem with the password in the documentation of the option, so people will be aware about the problem. The only sane solution is adding passwordFile support to the upstream icingaweb2, but I don't have time for that.

@aanderse
Copy link
Member

Understood. Yeah it isn't a trivial amount of work to put that in a preStart.

Looking good!

@dasJ
Copy link
Member Author

dasJ commented Apr 4, 2019

@infinisil Is there a way this will get into 19.03? Otherwise, I'd remove the module on the 19.03 branch so people don't start relying on a module that will change in the next release.

@infinisil infinisil merged commit fab50f0 into NixOS:master Apr 4, 2019
@infinisil
Copy link
Member

Backported in 8569d30 :)

@dasJ dasJ deleted the redo-icingaweb2 branch April 6, 2019 16:34
@dasJ
Copy link
Member Author

dasJ commented Apr 6, 2019

Thank you so much! :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants