refactor: DRY extensions NixOS test #1906
Conversation
|
there are three instances of |
|
thanks for explaining; makes sense! will review after conflicts are fixed. |
jfroche
force-pushed
the
chore/dry-nixos-tests
branch
from
625cec1 to
a4264ec
Compare
jfroche
force-pushed
the
chore/dry-nixos-tests
branch
2 times, most recently
from
4800989 to
89485d9
Compare
samrose
force-pushed
the
update-nixpkgs
branch
2 times, most recently
from
771718f to
4e32d5b
Compare
jfroche
force-pushed
the
update-nixpkgs
branch
from
acf3ba4 to
42b0c29
Compare
samrose
force-pushed
the
update-nixpkgs
branch
from
fa26b67 to
0131d17
Compare
Extract common NixOS test node configuration into common.nix to eliminate duplication across extension tests. Tests with custom requirements use lib.mkMerge to override defaults. This helps us to see at a glance why a test deviates from the standard setup.
jfroche
force-pushed
the
chore/dry-nixos-tests
branch
from
89485d9 to
c5cd83f
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (15)
WalkthroughRemoves automatic preloading of plpgsql/plpgsql_check and introduces a shared test library (nix/ext/tests/lib.nix). Many extension test files are refactored to import testLib and delegate node/postgres/extension wiring to it, replacing large inline test node and packaging logic. Changes
*Files listed are grouped—see individual test files for exact changes. Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/Test Runner
participant Lib as testLib (nix/ext/tests/lib.nix)
participant Nix as Nixpkgs (pkgs)
participant Node as NixOS Test Node
participant PG as PostgreSQL package
participant Ext as Extension (built)
Dev->>Lib: import ./lib.nix { testedExtensionName = pname }
Lib->>Nix: build/mkPostgresqlWithExtensions(psql_15/17)
Nix-->>PG: wrapped PostgreSQL derivation (with extension paths)
Dev->>Node: mkDefaultNixosTestNode { psql_15 psql_17 }
Node->>PG: instantiate and start Postgres
PG->>Ext: load extension libraries (when configured)
Node->>Dev: run testScript (pg_regress / migrations / upgrade checks)
(Note: rectangles use default styling; diagram focuses on high-level sequence.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jfroche
force-pushed
the
chore/dry-nixos-tests
branch
from
c5cd83f to
9269508
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/ext/tests/lib.nix`:
- Around line 83-118: The firewall and migration code dereference optional
settings directly; update networking.firewall.allowedTCPPorts to use a safe
default like (config.services.postgresql.settings.port or 5432) instead of
directly accessing config.services.postgresql.settings.port, and simplify the
shared_preload_libraries handling in the postgresql-migrate script by binding a
local variable (e.g. let spl =
config.services.postgresql.settings.shared_preload_libraries or null) and then
using if spl != null then include the --old-options/--new-options string with
${spl} else an empty string; change references in the
specialisation.postgresql17 block (variables newPostgresql/oldPostgresql,
oldDataDir/newDataDir, and the interpolation that currently checks
config.services.postgresql.settings.shared_preload_libraries) to use that
guarded value.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
nix/ext/tests/default.nixnix/ext/tests/http.nixnix/ext/tests/lib.nixnix/ext/tests/orioledb.nixnix/ext/tests/pg_repack.nixnix/ext/tests/pgjwt.nixnix/ext/tests/pgmq.nixnix/ext/tests/pgroonga.nixnix/ext/tests/pgrouting.nixnix/ext/tests/pgsodium.nixnix/ext/tests/plpgsql_check.nixnix/ext/tests/plv8.nixnix/ext/tests/postgis.nixnix/ext/tests/timescaledb.nixnix/ext/tests/vault.nix
💤 Files with no reviewable changes (2)
- nix/ext/tests/pgjwt.nix
- nix/ext/tests/orioledb.nix
🚧 Files skipped from review as they are similar to previous changes (2)
- nix/ext/tests/http.nix
- nix/ext/tests/pg_repack.nix
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nix-eval / eval
🔇 Additional comments (31)
nix/ext/tests/lib.nix (3)
31-41: LGTM: installedExtension mapping is clear and direct.
136-179: Nice consolidation of extension packaging logic.
180-189: LGTM for the versioned helpers.nix/ext/tests/postgis.nix (2)
4-13: Clean adoption of testLib for node wiring.
21-22: LGTM: version list construction is consistent.nix/ext/tests/timescaledb.nix (3)
4-9: LGTM: testLib wiring is straightforward.
15-26: Good use of mkMerge with a focused override.
38-38: LGTM for version list update.nix/ext/tests/plv8.nix (3)
5-10: LGTM: testLib bindings are clear.
14-14: LGTM: node definition is concise.
20-20: LGTM for version list update.nix/ext/tests/vault.nix (3)
4-16: LGTM: extension packaging with pgsodium is clean.
28-70: Override of pg17 migration script looks well-scoped.
79-90: LGTM: version table + metadata are consistent with testLib.nix/ext/tests/pgroonga.nix (3)
4-13: Clean testLib integration.The import and inherit pattern properly centralizes shared configuration while keeping the pgroonga-specific setup separate. The explicit listing of inherited values (
versions,installedExtension,psql_15,psql_17) improves readability and makes dependencies clear.
20-32: Good use ofmkMergeto extend the default node.Using
pkgs.lib.mkMergeto layer pgroonga-specific environment variables (MECAB, GRN settings) on top of the commonmkDefaultNixosTestNodeis the right approach—it maintains extension-specific requirements while benefiting from the DRY refactor.
42-43: Version mapping correctly wired.The
versionsfunction from testLib is properly invoked with the PostgreSQL version string to generate the Python list.nix/ext/tests/default.nix (3)
5-7: Correct exclusion oflib.nixfrom test file discovery.Adding
lib.nixto the filter prevents the shared library from being mistakenly processed as a test file. This is necessary given lib.nix is in the same directory.
12-17: testLib integration in generic test builder looks correct.The
extTestfunction properly imports testLib with the dynamicextension_nameparameter and inherits the required values. This allows the generic test builder to use the same centralized configuration as custom test files.
22-22: Simplified node definition.The single-line node definition using
testLib.mkDefaultNixosTestNodeis concise and consistent with the DRY objective.nix/ext/tests/plpgsql_check.nix (2)
4-18: Consistent testLib adoption.The testLib import, inherit block, and
mkDefaultNixosTestNodeusage follow the same pattern as other refactored test files, successfully centralizing common configuration.
27-28: Version mapping consistent with other files.Using
pkgs.lib.concatStringsSepaligns with the convention established across the refactored test files.nix/ext/tests/pgmq.nix (2)
4-18: testLib integration follows established pattern.The import, inherit block, and node definition are consistent with other refactored files. The extension-specific behavior (custom after-create.sql scripts) is properly preserved in the testScript.
27-28: Version mapping correct.Consistent use of
pkgs.lib.concatStringsSepfor generating Python version lists.nix/ext/tests/pgrouting.nix (3)
7-23: Custom PostgreSQL wiring for postgis dependency.pgrouting requires postgis, so it correctly uses
mkPostgresqlWithExtensionsto create custom psql_* packages with both extensions. This is the right approach when an extension has dependencies beyond what the default testLib provides.
30-78:mkMergewith orioledb17 specialisation is well-structured.The merge layers the orioledb-17 specialisation on top of the default node. The
postgresql-migrateservice that removes the datadir is documented with a clear comment explaining why migration isn't supported—this is intentional for the test environment.
87-89: Version mappings include orioledb-17 variant.Correctly generates version lists for all three PostgreSQL variants (15, 17, orioledb-17) that pgrouting tests against.
nix/ext/tests/pgsodium.nix (4)
4-16: LGTM!The testLib integration follows the established pattern from other refactored test files. The explicit inclusion of
hypopgalongsidepnamesuggests pgsodium tests depend on hypopg functionality.
18-22: LGTM!Using
pkgs.lib.getExeis the idiomatic way to obtain the executable path, and the 256-bit dummy key is appropriate for test purposes.
27-64: LGTM!The
mkMergeapproach cleanly combines the shared testLib defaults with pgsodium-specific requirements. The custom migration script is appropriately defined here (rather than in lib.nix) since pgsodium requires the additionalpgsodium.getkey_scriptconfiguration in postgresql.conf during upgrade, which differs from the generic migration logic.
65-111: LGTM!The test script comprehensively validates:
- Incremental extension upgrade paths through all versions on PostgreSQL 15
- Clean installation of the latest extension version
- PostgreSQL 15 to 17 migration via specialisation switching
- Upgrade paths on PostgreSQL 17 post-migration
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| networking.firewall.allowedTCPPorts = [ config.services.postgresql.settings.port ]; | ||
|
|
||
| specialisation.postgresql17.configuration = pkgs.lib.mkIf (psql_17 != null) { | ||
| services.postgresql = { | ||
| package = pkgs.lib.mkForce psql_17; | ||
| settings = (installedExtension "17").defaultSettings or { }; | ||
| }; | ||
|
|
||
| systemd.services.postgresql-migrate = { | ||
| serviceConfig = { | ||
| Type = "oneshot"; | ||
| RemainAfterExit = true; | ||
| User = "postgres"; | ||
| Group = "postgres"; | ||
| StateDirectory = "postgresql"; | ||
| WorkingDirectory = "${builtins.dirOf config.services.postgresql.dataDir}"; | ||
| }; | ||
| script = | ||
| let | ||
| oldPostgresql = psql_15; | ||
| newPostgresql = psql_17; | ||
| oldDataDir = "${builtins.dirOf config.services.postgresql.dataDir}/${oldPostgresql.psqlSchema}"; | ||
| newDataDir = "${builtins.dirOf config.services.postgresql.dataDir}/${newPostgresql.psqlSchema}"; | ||
| in | ||
| '' | ||
| if [[ ! -d ${newDataDir} ]]; then | ||
| install -d -m 0700 -o postgres -g postgres "${newDataDir}" | ||
| ${newPostgresql}/bin/initdb -D "${newDataDir}" | ||
| ${newPostgresql}/bin/pg_upgrade --old-datadir "${oldDataDir}" --new-datadir "${newDataDir}" \ | ||
| --old-bindir "${oldPostgresql}/bin" --new-bindir "${newPostgresql}/bin" \ | ||
| ${ | ||
| if config.services.postgresql.settings.shared_preload_libraries != null then | ||
| " --old-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}' --new-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}'" | ||
| else | ||
| "" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file and lines exist
wc -l nix/ext/tests/lib.nix
echo "---"
# Look at the specific lines mentioned
sed -n '83,118p' nix/ext/tests/lib.nixRepository: supabase/postgres
Length of output: 1956
🏁 Script executed:
#!/bin/bash
# List tests using mkDefaultNixosTestNode and those setting shared_preload_libraries
echo "=== Tests using mkDefaultNixosTestNode ==="
rg -n "mkDefaultNixosTestNode" nix/ext/tests -g '*.nix' -A 5 -B 1 | head -50
echo ""
echo "=== Tests setting shared_preload_libraries ==="
rg -n "shared_preload_libraries" nix/ext/tests -g '*.nix' -B 3 -A 3Repository: supabase/postgres
Length of output: 8309
🏁 Script executed:
#!/bin/bash
# Check how mkDefaultNixosTestNode is defined and what defaults it sets
fd "lib.nix" nix/ext/tests --type f
echo "---"
# Search for mkDefaultNixosTestNode definition
rg -n "mkDefaultNixosTestNode" nix/ext/tests -g '*.nix' | head -20Repository: supabase/postgres
Length of output: 1774
🏁 Script executed:
#!/bin/bash
# Check the PostgreSQL settings and their defaults in the test config
rg -n "services.postgresql.settings" nix/ext/tests -g '*.nix' -B 2 -A 2 | head -100Repository: supabase/postgres
Length of output: 1663
🏁 Script executed:
#!/bin/bash
# View the mkDefaultNixosTestNode function definition
sed -n '54,82p' nix/ext/tests/lib.nixRepository: supabase/postgres
Length of output: 844
🏁 Script executed:
#!/bin/bash
# Check if settings have defaults or are initialized in mkDefaultNixosTestNode
sed -n '54,85p' nix/ext/tests/lib.nix | cat -nRepository: supabase/postgres
Length of output: 1243
🏁 Script executed:
#!/bin/bash
# Search for where PostgreSQL settings are initialized with defaults
rg -n "port.*5432\|port.*=" nix/ext/tests -g '*.nix' | head -20Repository: supabase/postgres
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find where installedExtension is defined
rg -n "installedExtension" nix/ext/tests/lib.nix -B 5 -A 10 | head -80Repository: supabase/postgres
Length of output: 3083
🏁 Script executed:
#!/bin/bash
# Look at the definition of installedExtension function
rg -n "installedExtension\s*=" nix/ext/tests/lib.nix -A 15Repository: supabase/postgres
Length of output: 772
🏁 Script executed:
#!/bin/bash
# Check if there's a defaultSettings defined anywhere for extensions
rg -n "defaultSettings" nix/ext/tests -g '*.nix' -B 2 -A 2Repository: supabase/postgres
Length of output: 2517
🏁 Script executed:
#!/bin/bash
# Search for port configuration or defaults in the test files
rg -n "port" nix/ext/tests/lib.nix -B 2 -A 2Repository: supabase/postgres
Length of output: 625
🏁 Script executed:
#!/antml:thinking
The user needs verification. Let me check what happens when you access a non-existent attribute in Nix. The key question is: will line 83 fail if port is not set?
In Nix, if you try to access an attribute that doesn't exist, you get an error "attribute 'port' missing". So if `config.services.postgresql.settings.port` is not defined, the evaluation will fail.
The fact that the code at line 114 has a null check suggests the reviewer is aware that these settings can be null or missing.
Let me verify if there are actual test failures or if this is a proactive fix.
</thinking
#!/bin/bash
# Check if there are any test results or CI logs showing failures
git log --oneline -20 -- nix/ext/tests/lib.nixRepository: supabase/postgres
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Search for how NixOS PostgreSQL module defines defaults
# Look for any mkDefault or defaults in the code
rg -n "mkDefault.*port\|defaultSettings\s*=" nix/ -g '*.nix' | head -20Repository: supabase/postgres
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check how extensions define defaultSettings
fd "default.nix" nix/ext -type f --exec grep -l "defaultSettings" {} \;Repository: supabase/postgres
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Look at the actual extension packages to see what defaultSettings they provide
find nix/ext -name "default.nix" -type f | head -5 | while read f; do
echo "=== $f ==="
grep -n "defaultSettings" "$f" -A 5 | head -15
doneRepository: supabase/postgres
Length of output: 738
🏁 Script executed:
#!/bin/bash
# Check NixOS PostgreSQL defaults by looking for where port is initialized
rg -n "services\.postgresql\." nix/ext/tests/lib.nix -A 1 | head -40Repository: supabase/postgres
Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Look at how NixOS postgres module defines defaults (check if we have access to nixpkgs reference)
# Also check if any test has defaultSettings with port
rg -n "defaultSettings.*port\|port.*defaultSettings\|port.*=" nix/ext -g '*.nix' -B 2 -A 2 | head -50Repository: supabase/postgres
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check recent git history for failures or related fixes
git log --all --oneline --grep="port\|firewall\|settings" -- nix/ext/tests/lib.nix | head -10Repository: supabase/postgres
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for any comments about port configuration
rg -n "5432\|firewall.*port" nix/ext/tests -g '*.nix' -B 3 -A 1Repository: supabase/postgres
Length of output: 43
Guard optional settings before dereference.
config.services.postgresql.settings.port is accessed without a guard at line 83 and will fail Nix evaluation if undefined. Additionally, the shared_preload_libraries attribute is handled defensively at line 114 but the conditional can be simplified. Add safe defaults/guards for both.
🛠️ Proposed fix
- networking.firewall.allowedTCPPorts = [ config.services.postgresql.settings.port ];
+ networking.firewall.allowedTCPPorts = [
+ (config.services.postgresql.settings.port or 5432)
+ ];
...
- ${
- if config.services.postgresql.settings.shared_preload_libraries != null then
- " --old-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}' --new-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}'"
- else
- ""
- }
+ ${pkgs.lib.optionalString
+ ((config.services.postgresql.settings.shared_preload_libraries or null) != null)
+ " --old-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}' --new-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}'"
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| networking.firewall.allowedTCPPorts = [ config.services.postgresql.settings.port ]; | |
| specialisation.postgresql17.configuration = pkgs.lib.mkIf (psql_17 != null) { | |
| services.postgresql = { | |
| package = pkgs.lib.mkForce psql_17; | |
| settings = (installedExtension "17").defaultSettings or { }; | |
| }; | |
| systemd.services.postgresql-migrate = { | |
| serviceConfig = { | |
| Type = "oneshot"; | |
| RemainAfterExit = true; | |
| User = "postgres"; | |
| Group = "postgres"; | |
| StateDirectory = "postgresql"; | |
| WorkingDirectory = "${builtins.dirOf config.services.postgresql.dataDir}"; | |
| }; | |
| script = | |
| let | |
| oldPostgresql = psql_15; | |
| newPostgresql = psql_17; | |
| oldDataDir = "${builtins.dirOf config.services.postgresql.dataDir}/${oldPostgresql.psqlSchema}"; | |
| newDataDir = "${builtins.dirOf config.services.postgresql.dataDir}/${newPostgresql.psqlSchema}"; | |
| in | |
| '' | |
| if [[ ! -d ${newDataDir} ]]; then | |
| install -d -m 0700 -o postgres -g postgres "${newDataDir}" | |
| ${newPostgresql}/bin/initdb -D "${newDataDir}" | |
| ${newPostgresql}/bin/pg_upgrade --old-datadir "${oldDataDir}" --new-datadir "${newDataDir}" \ | |
| --old-bindir "${oldPostgresql}/bin" --new-bindir "${newPostgresql}/bin" \ | |
| ${ | |
| if config.services.postgresql.settings.shared_preload_libraries != null then | |
| " --old-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}' --new-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}'" | |
| else | |
| "" | |
| } | |
| networking.firewall.allowedTCPPorts = [ | |
| (config.services.postgresql.settings.port or 5432) | |
| ]; | |
| specialisation.postgresql17.configuration = pkgs.lib.mkIf (psql_17 != null) { | |
| services.postgresql = { | |
| package = pkgs.lib.mkForce psql_17; | |
| settings = (installedExtension "17").defaultSettings or { }; | |
| }; | |
| systemd.services.postgresql-migrate = { | |
| serviceConfig = { | |
| Type = "oneshot"; | |
| RemainAfterExit = true; | |
| User = "postgres"; | |
| Group = "postgres"; | |
| StateDirectory = "postgresql"; | |
| WorkingDirectory = "${builtins.dirOf config.services.postgresql.dataDir}"; | |
| }; | |
| script = | |
| let | |
| oldPostgresql = psql_15; | |
| newPostgresql = psql_17; | |
| oldDataDir = "${builtins.dirOf config.services.postgresql.dataDir}/${oldPostgresql.psqlSchema}"; | |
| newDataDir = "${builtins.dirOf config.services.postgresql.dataDir}/${newPostgresql.psqlSchema}"; | |
| in | |
| '' | |
| if [[ ! -d ${newDataDir} ]]; then | |
| install -d -m 0700 -o postgres -g postgres "${newDataDir}" | |
| ${newPostgresql}/bin/initdb -D "${newDataDir}" | |
| ${newPostgresql}/bin/pg_upgrade --old-datadir "${oldDataDir}" --new-datadir "${newDataDir}" \ | |
| --old-bindir "${oldPostgresql}/bin" --new-bindir "${newPostgresql}/bin" \ | |
| ${pkgs.lib.optionalString | |
| ((config.services.postgresql.settings.shared_preload_libraries or null) != null) | |
| " --old-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}' --new-options='-c shared_preload_libraries=${config.services.postgresql.settings.shared_preload_libraries}'" | |
| } |
🤖 Prompt for AI Agents
In `@nix/ext/tests/lib.nix` around lines 83 - 118, The firewall and migration code
dereference optional settings directly; update
networking.firewall.allowedTCPPorts to use a safe default like
(config.services.postgresql.settings.port or 5432) instead of directly accessing
config.services.postgresql.settings.port, and simplify the
shared_preload_libraries handling in the postgresql-migrate script by binding a
local variable (e.g. let spl =
config.services.postgresql.settings.shared_preload_libraries or null) and then
using if spl != null then include the --old-options/--new-options string with
${spl} else an empty string; change references in the
specialisation.postgresql17 block (variables newPostgresql/oldPostgresql,
oldDataDir/newDataDir, and the interpolation that currently checks
config.services.postgresql.settings.shared_preload_libraries) to use that
guarded value.
Move `postgresqlWithExtension` and `versions` to the test lib to reduce code duplication across extension tests.
jfroche
force-pushed
the
chore/dry-nixos-tests
branch
from
9269508 to
b520cc5
Compare
Extract common NixOS test node configuration and functions into lib.nix to eliminate duplication across extension tests.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.