-
-
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
Enrich PostgreSQL ensure users and databases support #203474
base: master
Are you sure you want to change the base?
Conversation
397f1ce
to
d6512d5
Compare
Specifically, this commit makes it possible to set various database parameters beside just the name.
d6512d5
to
fc51c59
Compare
This reorganizes the ensure code generation to make the code a bit less dense and also do general cleanups. Most importantly, the order is changed to first create users, then databases, and finally grants. This allows the database creation to refer to new users as owners.
fc51c59
to
0135d1c
Compare
This replaces the explicit database setup script to instead use the PostgreSQL `ensureUsers` and `ensureDatabases` options.
0135d1c
to
18f3e9d
Compare
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.
Amazing! Do you think it would be possible to have a bit of more NixOS tests for exercising this? Do you know if this can affect existing setups, as in, have you tried to run other tests? Can you add release notes information on this?
@RaitoBezarius Thanks for having a look! Yeah, more tests would be good. Shouldn't be too tricky. The change should be entirely backwards compatible. I can add a bullet point in the release notes. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/anybody-wanna-adopt-203474/25524/1 |
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.
We are missing migrations from the old settings.
|
||
isTemplate = mkOption { | ||
type = types.nullOr types.bool; | ||
default = null; |
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.
Why not default to false here?
|
||
allowConnections = mkOption { | ||
type = types.nullOr types.bool; | ||
default = null; |
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 think I reasonable default would be true
default = null; | ||
example = "sv_SE.utf8"; | ||
description = lib.mdDoc '' | ||
Character classification to use for the new database. |
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.
Does this fallback to the current environment set locale?
|
||
template = mkOption { | ||
type = types.nullOr types.str; | ||
default = null; |
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.
What uses postgres when creating a DB? We should default to that
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 suspect that most of these options might have different defaults depending on the postgresql version.
Some of these could also be influenced by options for the daemon itself
I can confirm that the mechanism to set the database owner implemented here solves #216989. Looking forward to seeing this merged! Note that the manual section on Nextcloud should be updated to include setting the owner. |
@exzombie I will not work more on this PR. I would be happy for you to take it over if you are interested. |
@rycee I'm sorry to hear that. I'm afraid I can't take this over right now, too much going on. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/what-about-state-management/37082/1 |
Description of changes
This PR includes a general cleanup of the generation of the "ensure users and databases" script.
It also makes it possible to create users with password.
Finally, it also makes it possible to create databases with additional parameters such as DB owner and encoding.
The matrix-synapse test is updated to use the new capabilities.
The changes should be backwards compatible.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes