nixos/firefox-syncserver: add PostgreSQL backend support - #501943
Conversation
e4d6d84 to
8af1c5f
Compare
ntninja
left a comment
There was a problem hiding this comment.
Since my independently developed version #507460 of this change was deemed a duplicate of this, I gave your version a review with things that are better in my version (some is better in yours).
should we just compile for both mysql and PostgreSQL?
That is not possible, the source code is written in such a way that if you compile for both you’ll get a symbol clash.
For keeping having the PostgreSQL feature on SyncServer not enable MySQL on TokenServer, I have a small patch:
https://github.com/ntninja/nixpkgs/blob/8f8c170c8628ca864d41eb80078147f828cc37db/pkgs/by-name/sy/syncstorage-rs/01-syncserver-tokenserver-postgres-db.patch
Upstream is mostly happy with it, other than it breaking their CI script assumptions, will update soon hopefully:
mozilla-services/syncstorage-rs#2194
b2cc6cd to
9f03b92
Compare
|
Thank you for your further review @mweinelt, I'm on it! |
19bcaba to
05aa52d
Compare
05aa52d to
67d5e80
Compare
|
Rebased. |
|
I'm terribly sorry for the long wait. I want postgres support so much, but couldn't find the motivation to get to this PR quickly. |
|
@mweinelt No need to apologize. Thanks for the reviews, I will get to them soon! |
67d5e80 to
5fb2afd
Compare
|
rebased |
18d61ba to
c7525ea
Compare
|
Both the mysql and the postgresql NixOS tests time out for me. |
|
Weird, worked on my machine, maybe the rebase broke it. I will investigate when I get home. |
diff --git a/nixos/tests/firefox-syncserver.nix b/nixos/tests/firefox-syncserver.nix
index 89110928d1e9..2e03d8cd8f9b 100644
--- a/nixos/tests/firefox-syncserver.nix
+++ b/nixos/tests/firefox-syncserver.nix
@@ -9,7 +9,7 @@ let
{
name = "firefox-syncserver-${name}";
- nodes.machine =
+ containers.machine =
{ pkgs, ... }:
lib.mkMerge [
{
@@ -43,7 +43,7 @@ let
testScript = ''
machine.wait_for_unit("multi-user.target")
machine.wait_until_succeeds("systemctl is-active firefox-syncserver.service", timeout=120)
- machine.wait_until_succeeds("curl -s http://127.0.0.1:5000/__heartbeat__ | jq -e '.database == \"Ok\" and .status == \"OK\"'")
+ machine.wait_until_succeeds("curl -s http://127.0.0.1:5000/__heartbeat__ | jq -e '.database == \"Ok\" and .status == \"Ok\"'")
'';
}
);nspawn container tests and the test fix |
840cfc3 to
9a25fb8
Compare
Expose both database backends as top-level packages so Hydra builds and caches each variant. The firefox-syncserver module now picks the appropriate package based on `services.firefox-syncserver.database.type` when the user does not supply one explicitly. The `package` option becomes `nullOr package` with a `null` default; this avoids requiring a user-supplied custom package to implement the `dbBackend` override attribute.
9261067 to
c2c2e67
Compare
Added support to the firefox-syncserver module for PostgreSQL databases. The upstream project has support for it, but the module did not. I also modified the actual firefox-syncserver package to allow the PostgreSQL feature flag to be set by changing the target database backend.
I needed to bump the version of the underlying package as the version currently shipping doesn't contain PostgreSQL support.
One concern I have is about the feature flag handling, should we just compile for both mysql and PostgreSQL? Right now I have it so it's either-or and it's determined by a feature flag.
LLMs used: Claude Opus 4.6 via opencode
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.