TRT-2865: Support external Postgres DSN for integration tests - #3856
TRT-2865: Support external Postgres DSN for integration tests#3856mstaeble wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@mstaeble: This pull request references TRT-2865 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughIntegration tests now support PostgreSQL from either testcontainers or ChangesPostgreSQL integration testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant PostgreSQL
participant IntegrationTests
CI->>PostgreSQL: start temporary PostgreSQL 16 instance
CI->>IntegrationTests: export INTEGRATION_DATABASE_DSN
IntegrationTests->>PostgreSQL: create and clone randomized databases
IntegrationTests-->>CI: complete make integration
CI->>PostgreSQL: stop instance and remove temporary data
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 3 warnings)
✅ Passed checks (16 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Dockerfile.integration (1)
5-9: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument why
repo_gpgcheckis disabled.Disabling
repo_gpgcheckremoves GPG verification of the PGDG repository metadata itself, weakening supply-chain integrity for packages installed during the build. This is a documented workaround for known PGDG repository signature issues: "encounter the error of [Error -1] repomd.xml signature could not be verified for pgdg-common". Add a short comment stating why this is needed (the specific known issue) so a future maintainer does not remove it blindly or assume it is unnecessary hardening debt.As per coding guidelines, "Keep comments minimal and helpful, and make them explain the 'why' rather than the 'what'."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile.integration` around lines 5 - 9, Add a brief why-comment immediately before the repo_gpgcheck modification in the Dockerfile command, stating that disabling metadata verification works around the known PGDG “repomd.xml signature could not be verified for pgdg-common” error. Keep the comment minimal and leave the existing installation flow unchanged.Source: Coding guidelines
scripts/integration-ci.sh (1)
14-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRestrict trust auth to the Unix socket only.
initdb --auth=trustapplies trust authentication to bothlocal(Unix socket) and defaulthost(TCP loopback) entries inpg_hba.conf. Sincepg_ctl start -o "-k /tmp"does not disable TCP listening, this instance also accepts unauthenticated TCP connections on127.0.0.1. Since the DSN only uses the Unix socket (host=/tmp), disable TCP listening entirely to reduce the unauthenticated surface.🔒 Proposed fix
-pg_ctl start -D "$PGDATA" -o "-k /tmp" -w +pg_ctl start -D "$PGDATA" -o "-k /tmp -c listen_addresses=''" -w🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/integration-ci.sh` around lines 14 - 17, Update the PostgreSQL startup invocation using pg_ctl in scripts/integration-ci.sh to disable TCP listening entirely while preserving the Unix socket at /tmp. Ensure the integration DSN’s socket-based connection continues to work and unauthenticated trust access is not exposed over loopback TCP.test/integration/util/testdb.go (1)
179-180: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid building DDL identifiers with
fmt.Sprintf.OpenGrep and ast-grep both flag
fmt.Sprintf-built SQL passed toExecat these five locations. The interpolated values (templateDB,dbName) are generated internally as"tmpl_" + randomSuffix()or"test_" + randomSuffix(), so the immediate injection risk is low since they are not derived from external input. However,CREATE/ALTER/DROP DATABASEcannot use query placeholders for identifiers, so quote or validate the identifier explicitly (e.g.,pgx.Identifier{templateDB}.Sanitize()) instead of raw string interpolation, so the code does not silently become unsafe if a future change derives these names from external or test-supplied input.As per coding guidelines, "Do not build SQL queries by concatenating or formatting user input directly; use placeholders and prepared statements instead," and as per path instructions, "SQL: parameterized queries only; no string concatenation."
Also applies to: 195-195, 215-215, 234-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/util/testdb.go` around lines 179 - 180, Replace the fmt.Sprintf-built DDL statements passed to adminDB.Exec in the database setup/cleanup flow with explicitly sanitized PostgreSQL identifiers, such as pgx.Identifier{templateDB}.Sanitize() or the equivalent established helper. Update all affected CREATE, ALTER, and DROP DATABASE statements, including the locations using templateDB and dbName, while preserving their existing behavior and removing raw identifier interpolation.Sources: Coding guidelines, Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/util/testdb.go`:
- Around line 110-121: Update startExternalPostgres to validate that u.Scheme is
a supported Postgres scheme, such as postgres or postgresql, in addition to
rejecting an empty scheme. Return the existing clear DSN validation error for
unsupported schemes before calling createTemplateDB.
- Around line 173-182: Update dropTemplateDB to accept the context provided by
Terminate, use ExecContext for both database operations, and check the
IS_TEMPLATE reset error before attempting the drop. Wrap and return that error
with descriptive context, while preserving the existing drop error handling.
- Around line 184-220: Update createTemplateDB to clean up templateDB after
CREATE DATABASE succeeds but db.New, dbc.DB.DB(), or SetupIntegrationSchema
fails. Drop the database through adminDB before returning each setup error,
while preserving the original error context and existing successful flow.
---
Nitpick comments:
In `@Dockerfile.integration`:
- Around line 5-9: Add a brief why-comment immediately before the repo_gpgcheck
modification in the Dockerfile command, stating that disabling metadata
verification works around the known PGDG “repomd.xml signature could not be
verified for pgdg-common” error. Keep the comment minimal and leave the existing
installation flow unchanged.
In `@scripts/integration-ci.sh`:
- Around line 14-17: Update the PostgreSQL startup invocation using pg_ctl in
scripts/integration-ci.sh to disable TCP listening entirely while preserving the
Unix socket at /tmp. Ensure the integration DSN’s socket-based connection
continues to work and unauthenticated trust access is not exposed over loopback
TCP.
In `@test/integration/util/testdb.go`:
- Around line 179-180: Replace the fmt.Sprintf-built DDL statements passed to
adminDB.Exec in the database setup/cleanup flow with explicitly sanitized
PostgreSQL identifiers, such as pgx.Identifier{templateDB}.Sanitize() or the
equivalent established helper. Update all affected CREATE, ALTER, and DROP
DATABASE statements, including the locations using templateDB and dbName, while
preserving their existing behavior and removing raw identifier interpolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 42ab77d9-a7e8-479f-84c3-162caa7f970b
📒 Files selected for processing (5)
DEVELOPMENT.mdDockerfile.integrationMakefilescripts/integration-ci.shtest/integration/util/testdb.go
43f6e11 to
9f9998a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/integration/util/testdb.go (2)
202-224: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winComplete template cleanup on every setup failure.
Line 203 discards the
DROP DATABASEerror. Line 223 bypassescleanupOnErr. If either cleanup orALTER DATABASE ... IS_TEMPLATE = truefails, the function can leave an orphan template database on the external server.Pass
ctxintocreateTemplateDB, useExecContext, preserve cleanup failures with context, and route the Line 223 error throughcleanupOnErr.As per path instructions, “Never ignore error returns” and “context.Context for cancellation and timeouts.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/util/testdb.go` around lines 202 - 224, Update createTemplateDB to accept ctx and use ExecContext for database cleanup and ALTER DATABASE operations. In cleanupOnErr, capture DROP DATABASE failures and preserve them alongside the original setup error with contextual wrapping. Route the ALTER DATABASE failure through cleanupOnErr so every setup failure attempts cleanup without discarding errors.Sources: Coding guidelines, Path instructions
110-120: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd tests for external PostgreSQL lifecycle.
No unit tests cover
test/integration/util/testdb.go, so add coverage for invalidINTEGRATION_DATABASE_DSN, successful external template database cleanup, and cleanup failures forstartExternalPostgres,createTemplateDB, anddropTemplateDB.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/util/testdb.go` around lines 110 - 120, Add unit tests covering invalid DSNs in startExternalPostgres, successful external template database creation and cleanup across startExternalPostgres/createTemplateDB/dropTemplateDB, and cleanup-error propagation when createTemplateDB or dropTemplateDB fails. Use controllable test dependencies or fixtures to exercise each lifecycle path without requiring a live PostgreSQL instance.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/util/testdb.go`:
- Around line 110-114: Update the validation error in startExternalPostgres so
it no longer includes the raw dsn value, which may contain credentials. Return a
fixed message or safely redacted URL while preserving the existing invalid-URL
and missing-scheme validation behavior.
---
Outside diff comments:
In `@test/integration/util/testdb.go`:
- Around line 202-224: Update createTemplateDB to accept ctx and use ExecContext
for database cleanup and ALTER DATABASE operations. In cleanupOnErr, capture
DROP DATABASE failures and preserve them alongside the original setup error with
contextual wrapping. Route the ALTER DATABASE failure through cleanupOnErr so
every setup failure attempts cleanup without discarding errors.
- Around line 110-120: Add unit tests covering invalid DSNs in
startExternalPostgres, successful external template database creation and
cleanup across startExternalPostgres/createTemplateDB/dropTemplateDB, and
cleanup-error propagation when createTemplateDB or dropTemplateDB fails. Use
controllable test dependencies or fixtures to exercise each lifecycle path
without requiring a live PostgreSQL instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2681933d-341b-4cc1-ae22-e5b7868f95ab
📒 Files selected for processing (5)
DEVELOPMENT.mdDockerfile.integrationMakefilescripts/integration-ci.shtest/integration/util/testdb.go
💤 Files with no reviewable changes (1)
- Dockerfile.integration
🚧 Files skipped from review as they are similar to previous changes (3)
- DEVELOPMENT.md
- scripts/integration-ci.sh
- Makefile
9f9998a to
3f28ca1
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
3f28ca1 to
7b1424f
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7b1424f to
a32196b
Compare
|
Scheduling required tests: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
openshift-ci
Bot
added
the
ready-for-human-review
|
@mstaeble: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Exploring option of using nested-podman container, as shown in openshift/release#81145. |
openshift-ci
Bot
added
the
do-not-merge/hold
2 participants
Summary
INTEGRATION_DATABASE_DSNenvironment variable to run integration tests against an external PostgreSQL server instead of requiring testcontainers-go to spin up a container. This enables integration tests to run in CI environments (ci-operator) that lack a container runtime.Dockerfile.integrationthat layers PostgreSQL 16 on top of the build root image, andscripts/integration-ci.shthat starts a local Postgres instance and runs the tests.DEVELOPMENT.mdandMakefile.Test plan
Dockerfile.integrationusing the external DSN path🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation