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.

Sign up for GitHub

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

Jump to bottom

postgresql: Add ensureExtensions option to service setup #75688

Closed
wants to merge 1 commit into from
Closed

postgresql: Add ensureExtensions option to service setup #75688

wants to merge 1 commit into from

Conversation

kampka
Copy link
Contributor

@kampka kampka commented Dec 15, 2019

Motivation for this change

There are still a couple of services that create postgres extensions and databases in preStart,
usually relying on privilege escalation to do it. This change will allow us to simplify extension creation and unify these modules.

Heads up for potential reviewers: The postgresql tests suite currently fails because it also tests the pgbackup service. This service uses command line options that was removed in recent postgres versions. I'll try to address that issue in a separate PR.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @thoughtpolice

@aanderse
Copy link
Member

Currently there is some disagreement about how far down the ensure* path we want to go: #68353

There is also a PR which aims to implement this functionality in a more declarative way: #72365

It might be more efficient to close this PR and work together with other community members working on this topic. Your call.

@kampka
Copy link
Contributor Author

kampka commented Dec 15, 2019 •
edited

Currently there is some disagreement about how far down the ensure* path we want to go: #68353

Well, I am definitely on the pro-ensure side of the discussion, specifically because services need to handle privilege escalation escalation to setup their databases / users / extensions / etc. Not having this will probably lead to more services having <service>-setup.service jobs in the future which will have to be maintained everywhere for compatibility.

There is also a PR which aims to implement this functionality in a more declarative way: #72365
It might be more efficient to close this PR and work together with other community members working on this topic. Your call.

I have left some comments on that PR. Generally, I am not against collaborating on this topic.
However, that issue is already pending for a couple of month now, mainly because of "political taint".
Since I have no personal stake in this issue other than further cleaning up the codebase, I have no intention of pushing it through atm. so closing it probably makes sense.

@kampka kampka closed this Dec 15, 2019
@aanderse aanderse mentioned this pull request Jan 21, 2020
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice thoughtpolice is a code owner

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kampka @aanderse