nixos/postgresql: check config file syntax at build time #92929
Conversation
|
@GrahamcOfBorg build nixosTests.postgresql |
| @@ -23,6 +23,11 @@ let | |||
| ${cfg.extraConfig} | |||
| ''; | |||
|
|
|||
| configFileCheck = pkgs.runCommand "postgresql-configfile-check" {} '' | |||
| ${cfg.package}/bin/postgres -D${configFile} -C config_file >/dev/null | |||
There was a problem hiding this comment.
See also #62853 for cross-compile support.
There was a problem hiding this comment.
this approach does not work so well for postgresql because the right version of the package should be used...
| @@ -279,6 +290,8 @@ in | |||
| "/share/postgresql" | |||
| ]; | |||
|
|
|||
| system.extraDependencies = lib.optional (cfg.checkConfig && pkgs.stdenv.hostPlatform == pkgs.stdenv.buildPlatform) configFileCheck; | |||
There was a problem hiding this comment.
It's not good UX to silently skip the syntax check when cross-building a system and having checkConfig set to true. In that case an assertion should be thrown IMHO.
There was a problem hiding this comment.
I copied the semantics of nix.checkConfig.
https://github.com/NixOS/nixpkgs/blob/release-20.03/nixos/modules/services/misc/nix-daemon.nix#L60
I don't find it good ux that this module throws an assertion error by default when cross compiling.
ofborg
bot
added
6.topic: nixos
symphorien
force-pushed
the
postgresql-check-config-file
branch
from
523baf4 to
1b7ca69
Compare
|
I fixed the merge conflict |
|
This seems like a larger problem that should be addressed more generally ... the only problem being that I have no idea how 🤔 It is entirely conceivable that a package doesn't have an equivalent on a different platform, or is broken on another platform, has a different feature set on a different platform, etc... I'm not sure we should assume this will even work in a cross compile/platform scenario (generally speaking). I think further discussion is required here. Or at least I'd like to know more before we proceed because I don't know enough about cross compilation to have any solid answers. |
I don't think that's a good idea: you have very specific tools where you sometimes have to hack around on your own (e.g. for the sshd validation I had to generate a temporary keypair in the openssh module), so I don't see how a general approach is really possible. |
|
I also agree that there is probably no good solution for cross, this is why this PR disables it for cross altogether. But for the easy-case (non-cross) I think checking the configuration file is a net plus. |
|
Are there a small number of entries in this config file which can contain reference to an external file? |
|
I don't know for sure, but at least it's possible to include configuration files outside of the store. https://www.postgresql.org/docs/9.3/config-setting.html |
Sure, we can't standardize to the degree of a specific implementation, but at a high level how can we do things consistently? I think @symphorien has a fantastic point in their comment: #92929 (comment) If we use that approach as a standard way to approach the problem at least users can have consistent expectations. |
|
A lot has been said, but I don't know how to make this progress forward, and I'd like to get this in before 21.05... |
|
I've stated my opinion... in that I agree with your opinion: ignore checking on cross compile and print a warning. That seems like the most reasonable and scales as a generic (enough) solution we can use anywhere. If @Ma27 didn't tell me not to then I'd merge that. 🤷♂️ |
|
Well I'll merge in a few days unless someone speaks up. |
|
@GrahamcOfBorg test postgresql postgresql-wal-receiver |
Motivation for this change
Don't crash postgresql when there is a typo in
extraConfigThings done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)