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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/postgresql: turn settings into a submodule #296616

Merged
merged 1 commit into from Apr 1, 2024

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 17, 2024

Description of changes

The main idea behind that was to be able to do more sophisticated merging for stuff that goes into postgresql.conf: shared_preload_libraries is a comma-separated list in a types.str and thus not mergeable. With this change, the option accepts both a comma-separated string xor a list of strings.

This can be implemented rather quick using coercedTo + freeform modules. The interface still behaves equally, but it allows to merge declarations for this option together.

One side-effect was that I had to change the attrsOf (oneOf ...) part into a submodule to allow declaring options for certain things. While at it, I decided to move log_line_prefix and port into this structure as well.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 馃憤 reaction to pull requests you find important.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3674

nixos/modules/services/misc/forgejo.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/gitea.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

I like the change and I also prefer the Sandro's suggestion. It's shorter and easier to reason about.

@Ma27 Can you merge them? So we can proceed?

@Ma27 Ma27 force-pushed the postgresql-rfc42-submodule branch from 16ba914 to 00f7f15 Compare March 27, 2024 11:55
@Ma27
Copy link
Member Author

Ma27 commented Mar 27, 2024

I don't think it makes that much of a difference (and is also not related to the diff), but I won't die on this hill. Updated accordingly.

The main idea behind that was to be able to do more sophisticated
merging for stuff that goes into `postgresql.conf`:
`shared_preload_libraries` is a comma-separated list in a `types.str`
and thus not mergeable. With this change, the option accepts both a
comma-separated string xor a list of strings.

This can be implemented rather quick using `coercedTo` +
freeform modules. The interface still behaves equally, but it allows to
merge declarations for this option together.

One side-effect was that I had to change the `attrsOf (oneOf ...)` part into
a submodule to allow declaring options for certain things. While at it,
I decided to move `log_line_prefix` and `port` into this structure as
well.
@Ma27 Ma27 force-pushed the postgresql-rfc42-submodule branch from 00f7f15 to 5142b7a Compare March 30, 2024 13:23
@Ma27
Copy link
Member Author

Ma27 commented Mar 30, 2024

Rebased onto latest master and resolved the merge conflict. Any final comments?

@bendlas
Copy link
Contributor

bendlas commented Mar 31, 2024

Is this a breaking change? It looks like it renames a couple of options without forwarding ...

@Ma27
Copy link
Member Author

Ma27 commented Mar 31, 2024

@bendlas which options specifically? Unless I'm missing something, it isn't.

I see that the manual build fails now though, will fix that soonish.

@bendlas
Copy link
Contributor

bendlas commented Mar 31, 2024

@Ma27 maybe I'm being dense, but what happens if somebody is running a configuration with services.postgresql.port = ...;?

@Ma27
Copy link
Member Author

Ma27 commented Mar 31, 2024

mkRenamedOptionModule makes sure that settings.port is set then and yields a warning. Declared here: https://github.com/NixOS/nixpkgs/pull/296616/files#diff-1eb0d5f14641b3640a20d71e7d4e1a9b9efcca7b152f5115f4029651bcff5817R46

@bendlas
Copy link
Contributor

bendlas commented Mar 31, 2024

OK, thanks, I missed that

@Ma27
Copy link
Member Author

Ma27 commented Mar 31, 2024

OK no idea what's up with the manual build, seems as if some cachix steps are failing in the GH action stuff here. Rerunning for now.

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.

LGTM. Thank you!

@marsam marsam merged commit 5b3704b into NixOS:master Apr 1, 2024
21 checks passed
@Ma27 Ma27 deleted the postgresql-rfc42-submodule branch April 2, 2024 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants