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/postgresql: enrich ensure users and databases support #255961

Closed

Conversation

Scrumplex
Copy link
Member

@Scrumplex Scrumplex commented Sep 18, 2023

Description of changes

Based on #203474

TODOs

  • Update release notes
  • Add tests for ensureClauses

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot 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/` labels Sep 18, 2023
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 18, 2023
@Scrumplex Scrumplex force-pushed the nixos/postgresql/better-ensure branch 7 times, most recently from 018e44e to fb401ad Compare September 18, 2023 21:04
@nixos-discourse
Copy link

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/2

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Sep 18, 2023
@Scrumplex Scrumplex changed the title postgresql: enrich ensure users and databases support nixos/postgresql: enrich ensure users and databases support Sep 20, 2023
Comment on lines +36 to +50
ensureDatabases = [
{
name = "hedgedocdb";
owner = "hedgedoc";
}
];
ensureUsers = [
{
name = "hedgedoc";
passwordFile = pkgs.writeText "hedgedoc-password" "snakeoilpassword";
ensureClauses.login = true;
}
];
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why the dbname and owner don't match here and why not socket auth is used but I also don't have enough time to also maintain the tests of my packages. sigh. :(

Copy link
Contributor

@marsam marsam left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks a lot!

# command line options.
psqlCmd = cmd: args: concatStringsSep " " (
[ "$PSQL -tA" ]
++ args
Copy link
Contributor

Choose a reason for hiding this comment

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

args doesn't seem to be used. What do you think of removing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

args is actually used a few lines below in ensureUsers

@marsam
Copy link
Contributor

marsam commented Oct 25, 2023

@GrahamcOfBorg test postgresql postgresql-ensure

rycee and others added 2 commits October 25, 2023 19:12
Specifically, this commit makes it possible to set various database
parameters beside just the name.
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 clauses and grants. This allows the database
creation to refer to new users as owners.

Co-authored-by: Sefa Eyeoglu <[email protected]>
Signed-off-by: Sefa Eyeoglu <[email protected]>
@bendlas
Copy link
Contributor

bendlas commented Nov 14, 2023

Unfortunately, It looks like this will a bit late for helping us adapt to postgres 15 for 23.11. For now, we'll deprecate ensurePermissions and instead advise for ensureDBOwnership + the occasional raw postStart script in the manual.

I would very much like to give a proper look at this approach for 24.05, especially also look at ways this could tie into system - wide approach towards state management and migrations. Also I'd like to look for a way to get the benefits of ensure-style options, while avoiding the pitfalls of convergent configuration management

I wrote out my initial thoughts here: #267365

@Scrumplex
Copy link
Member Author

I'll close this as I don't really intend on working on this much further. Though feel free to continue this, as we don't have an alternative to convergent configuration management for databases in Nixpkgs atm.

@Scrumplex Scrumplex closed this Dec 19, 2023
@nixos-discourse
Copy link

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

@Scrumplex Scrumplex deleted the nixos/postgresql/better-ensure branch September 29, 2024 08:59
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: changelog 8.has: documentation 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 needs_reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants