-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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/postgresql: allow specifying multiple databases in ensureDBOwnership #287788
base: master
Are you sure you want to change the base?
nixos/postgresql: allow specifying multiple databases in ensureDBOwnership #287788
Conversation
FYI, there are related discussions in #266270 and you can request people interested to review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I had in mind, so let me approve the PR :-).
Tiny nit to DRY it up.
assertions = map ({ name, ensureDBOwnership, ... }: { | ||
assertion = ensureDBOwnership -> builtins.elem name cfg.ensureDatabases; | ||
assertions = map ({ name, ensureDBOwnership, ... }: let | ||
dbOwnershipList = if builtins.isList ensureDBOwnership then ensureDBOwnership else if ensureDBOwnership then [name] else []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than repeat this multiple time, you can use apply
in the option definition to tranform a true
into a singleton list [ name ]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that but couldn't figure out how to use the value of name
within the apply
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types.submodule
can use an argument list which would allow you to get at the name IIRC.
Currently on my phone so it's difficult to check.
4fa080e
to
2e12bca
Compare
So I happen to have a database owned by a differently-named user and the database has data in it. Now I understand that I can just comment out the whole bit and the existing database will not be impacted, but I'd rather modernize the config properly, so: what's up with this PR? It's been quiet for a month, how do the prospects look? |
2e12bca
to
59aabe8
Compare
in concatStringsSep "\n" | ||
(builtins.map (db: ''$PSQL -tAc 'ALTER DATABASE "${db}" OWNER TO "${user.name}";' '') dbOwnershipList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in concatStringsSep "\n" | |
(builtins.map (db: ''$PSQL -tAc 'ALTER DATABASE "${db}" OWNER TO "${user.name}";' '') dbOwnershipList); | |
in concatMapStringsSep "\n" | |
(db: ''$PSQL -tAc 'ALTER DATABASE "${db}" OWNER TO "${user.name}";' '') dbOwnershipList; |
Description of changes
Allow settings
services.postgresql.ensureUsers.*.ensureDBOwnership
to a list of database names instead of only booleans.This allows using it for setups where the database name doesn't match the user name or there are multiple databases accessed by a single db user.
This should make
ensureDBOwnership
able to cover more use cases from the deprecatedensurePermissions
option.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.