feat(testing): add PostgreSQL support for unit tests#449
feat(testing): add PostgreSQL support for unit tests#449Hyperkid123 merged 4 commits intoRedHatInsights:mainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR extends the testing infrastructure to support both SQLite and PostgreSQL databases. It introduces PostgreSQL sidecar support in the Tekton pipeline, adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 15 minutes and 4 seconds.Comment |
Hyperkid123
left a comment
Hyperkid123
left a comment
There was a problem hiding this comment.
We want to run the PostgreSQL DB also in Konflux. You need to set up a sidecar in the Tekton workflow.
It was already done in the Chrome service. It is using a pipeline from https://github.com/RedHatInsights/konflux-pipelines/blob/main/pipelines/docker-build-run-unit-tests-dynamic-env.yaml. You can check that if it works, use it, if not, add the sidecar to the quickstarts Tekton directly.
|
Thanks for the pointer! I've switched the PR pipeline from The Unleash params are included with dummy tokens since the pipeline requires them (quickstarts doesn't use Unleash, so they're inert). |
|
@platex-rehor-bot there is an issue with dependencies: See if you can update go. It should not be an issue. If it still does not work, we may have to create a custom pipeline. |
|
Thanks for the heads up! The Go version mismatch was caused by the pipeline running Go 1.25.8 with I've added If the pipeline doesn't have network access to download toolchains, we may need to look at a custom pipeline or bump the base image instead. |
|
The previous CI run showed that Root cause: Fixes in this push (de45307):
|
|
@platex-rehor-bot we have this error in actual tests: |
|
The CI failure was a Go toolchain version mismatch: Root cause: Fix (8c659e6): Changed to |
|
So we no longer have the toocahing issue however there are the failing tests: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/testing-guidelines.md`:
- Around line 47-50: Update the example test command so it forces serial package
execution to avoid shared PostgreSQL races: change the shown `go test ./... -v`
usage to include the `-p 1` flag (e.g., `go test -p 1 ./... -v`) or replace the
example with a call to the existing `make test-pg` target; ensure the docs
reference the `TEST_DATABASE_URL` environment variable shown and explicitly
recommend `-p 1` or `make test-pg` to prevent concurrent package runs against
the shared DB.
In `@pkg/database/db.go`:
- Around line 103-120: Make CleanTestTables test-only and fail fast by changing
its signature to return an error and adding prechecks: verify config.Get().Test
is true and DB != nil before doing anything; if either check fails return a
descriptive error. Keep the PostgreSQL dialector check (DB.Dialector.Name() !=
"postgres") but return nil or an appropriate no-op error instead of silently
returning. Replace the current DB.Exec calls in the loop with code that captures
and returns any error immediately (include table name in the error message) so
callers can stop test setup on partial cleanup failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9f6870ca-ab96-492c-a9fa-4aa92cf526db
📒 Files selected for processing (10)
.tekton/quickstarts-pull-request.yamlAGENTS.mdMakefileconfig/config.godocs/testing-guidelines.mdlocal/db-compose.yamllocal/init-test-db.shpkg/database/db.gopkg/database/main_test.gopkg/routes/main_test.go
|
Good catch! The test failures are caused by a GORM migration ordering issue — not related to the toolchain fix. Root cause: GORM's Fix (6b93e6c): Reordered all
This ensures the |
|
We still hvae issues with the DB preparation. Can you call |
platex-rehor-bot
left a comment
platex-rehor-bot
left a comment
There was a problem hiding this comment.
Thanks for the pointer — the PostgreSQL sidecar is already configured in .tekton/quickstarts-pull-request.yaml using the same shared pipeline (docker-build-run-unit-tests-dynamic-env.yaml) and the same pattern as chrome-service-backend. The PG connection parameters (PGSQL_USER, PGSQL_PASSWORD, etc.) are all wired up, and the unit-tests-script waits for the sidecar to be ready before running tests.
The run-unit-tests CI failure turned out to be caused by three test assertions in pkg/database/db_test.go that compared exact SQLite-specific error messages (e.g. "sql: converting argument $4 type: invalid tag value" and "NOT NULL constraint failed: tags.value"). PostgreSQL returns different error strings for the same constraint violations, so these assertions failed.
Fixed in the latest commit by replacing the exact string matches with assert.Error + assert.Contains — the tests now validate that the correct errors occur without depending on a specific database driver's error format.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.tekton/quickstarts-pull-request.yaml (2)
33-50: Consider using secrets for database credentials.The PostgreSQL credentials (
PGSQL_USER,PGSQL_PASSWORD) and Unleash tokens are hardcoded in the PipelineRun. While these are test credentials for a local sidecar, consider using Kubernetes secrets for better security hygiene, especially since this file is committed to version control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/quickstarts-pull-request.yaml around lines 33 - 50, Replace hardcoded credentials in the PipelineRun env block (PGSQL_USER, PGSQL_PASSWORD, PGSQL_HOSTNAME, PGSQL_PORT, PGSQL_DATABASE, UNLEASH_API_TOKEN, UNLEASH_ADMIN_TOKEN and the env-vars TEST_DATABASE_URL) with references to Kubernetes Secrets using valueFrom/secretKeyRef (or envFrom) and update the pipeline/container spec to consume those secret keys; create a corresponding Secret resource (containing keys for pg_user, pg_password, pg_hostname, pg_port, pg_database, unleash_api_token, unleash_admin_token, and TEST_DATABASE_URL) and ensure the env-vars entry uses the secret value instead of inline TEST_DATABASE_URL so credentials are no longer stored directly in the YAML.
64-73: Consider usingpg_isreadyfor more robust PostgreSQL readiness check.The TCP port check (
/dev/tcp/...) only confirms the port is listening, not that PostgreSQL is ready to accept connections. Sincepsqlis available (used on Line 76), consider usingpg_isreadywhich checks actual PostgreSQL readiness.Suggested improvement
# Wait for PostgreSQL sidecar to be ready echo "Waiting for PostgreSQL..." for i in $(seq 1 30); do - if bash -c "echo > /dev/tcp/$(params.PGSQL_HOSTNAME)/$(params.PGSQL_PORT)" 2>/dev/null; then + if pg_isready -h $(params.PGSQL_HOSTNAME) -p $(params.PGSQL_PORT) -U $(params.PGSQL_USER) -q; then echo "PostgreSQL is ready" break fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/quickstarts-pull-request.yaml around lines 64 - 73, The current readiness loop uses a raw TCP check against $(params.PGSQL_HOSTNAME)/$(params.PGSQL_PORT) which only verifies a listening port; replace it with a loop that calls pg_isready -h "$PGSQL_HOSTNAME" -p "$PGSQL_PORT" (or the equivalent with $(params.PGSQL_HOSTNAME) and $(params.PGSQL_PORT)) until it reports the server as accepting connections, keeping the same retry count/sleep behavior as the existing for-loop; if pg_isready is unavailable, keep the original /dev/tcp fallback but prefer pg_isready for correctness and ensure the script still echoes attempts and the final "PostgreSQL is ready" message.pkg/database/main_test.go (1)
46-46: Consider addingFavoriteQuickstartfor consistency with routes tests.The
AutoMigratecall here excludesmodels.FavoriteQuickstart{}, butpkg/routes/main_test.goincludes it. While this may work if no database package tests useFavoriteQuickstart, the inconsistency could cause subtle failures later.Suggested alignment
- err = DB.AutoMigrate(&models.Tag{}, &models.Quickstart{}, &models.QuickstartProgress{}, &models.HelpTopic{}) + err = DB.AutoMigrate(&models.Tag{}, &models.Quickstart{}, &models.QuickstartProgress{}, &models.HelpTopic{}, &models.FavoriteQuickstart{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/database/main_test.go` at line 46, The AutoMigrate call in DB.AutoMigrate currently omits models.FavoriteQuickstart causing inconsistency with pkg/routes tests; update the AutoMigrate invocation (the DB.AutoMigrate(...) call) to include &models.FavoriteQuickstart{} alongside &models.Tag{}, &models.Quickstart{}, &models.QuickstartProgress{}, &models.HelpTopic{} so the FavoriteQuickstart schema is created for tests and kept consistent with routes tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.tekton/quickstarts-pull-request.yaml:
- Around line 75-76: Fix the typo in the comment above the psql command: change
the comment text "# veciry connection and active DBs" to "# verify connection
and active DBs" so the comment accurately describes the following psql
invocation (the line beginning with "psql -U $(params.PGSQL_USER) -h
$(params.PGSQL_HOSTNAME) -p $(params.PGSQL_PORT) -d $(params.PGSQL_DATABASE) -c
\"\\l\"").
---
Nitpick comments:
In @.tekton/quickstarts-pull-request.yaml:
- Around line 33-50: Replace hardcoded credentials in the PipelineRun env block
(PGSQL_USER, PGSQL_PASSWORD, PGSQL_HOSTNAME, PGSQL_PORT, PGSQL_DATABASE,
UNLEASH_API_TOKEN, UNLEASH_ADMIN_TOKEN and the env-vars TEST_DATABASE_URL) with
references to Kubernetes Secrets using valueFrom/secretKeyRef (or envFrom) and
update the pipeline/container spec to consume those secret keys; create a
corresponding Secret resource (containing keys for pg_user, pg_password,
pg_hostname, pg_port, pg_database, unleash_api_token, unleash_admin_token, and
TEST_DATABASE_URL) and ensure the env-vars entry uses the secret value instead
of inline TEST_DATABASE_URL so credentials are no longer stored directly in the
YAML.
- Around line 64-73: The current readiness loop uses a raw TCP check against
$(params.PGSQL_HOSTNAME)/$(params.PGSQL_PORT) which only verifies a listening
port; replace it with a loop that calls pg_isready -h "$PGSQL_HOSTNAME" -p
"$PGSQL_PORT" (or the equivalent with $(params.PGSQL_HOSTNAME) and
$(params.PGSQL_PORT)) until it reports the server as accepting connections,
keeping the same retry count/sleep behavior as the existing for-loop; if
pg_isready is unavailable, keep the original /dev/tcp fallback but prefer
pg_isready for correctness and ensure the script still echoes attempts and the
final "PostgreSQL is ready" message.
In `@pkg/database/main_test.go`:
- Line 46: The AutoMigrate call in DB.AutoMigrate currently omits
models.FavoriteQuickstart causing inconsistency with pkg/routes tests; update
the AutoMigrate invocation (the DB.AutoMigrate(...) call) to include
&models.FavoriteQuickstart{} alongside &models.Tag{}, &models.Quickstart{},
&models.QuickstartProgress{}, &models.HelpTopic{} so the FavoriteQuickstart
schema is created for tests and kept consistent with routes tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: bbc90609-9e44-4dd1-9f6c-738adfee5c2e
📒 Files selected for processing (7)
.tekton/quickstarts-pull-request.yamlcmd/migrate/migrate.godocs/testing-guidelines.mdpkg/database/db.gopkg/database/db_test.gopkg/database/main_test.gopkg/routes/main_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/migrate/migrate.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/routes/main_test.go
- docs/testing-guidelines.md
Hyperkid123
force-pushed
the
bot/RHCLOUD-42334
branch
8 times, most recently
from
da15784 to
098229f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/database/db.go (1)
89-105: Consider addingQuickstartProgressto the table creation sequence.The
Init()function createsTag,Quickstart,HelpTopic, andFavoriteQuickstarttables, butQuickstartProgressis missing here while it's included in the test setup'sAutoMigratecall. This inconsistency could cause issues ifInit()is called standalone without a subsequentAutoMigrate.Proposed addition
if !DB.Migrator().HasTable(&models.FavoriteQuickstart{}) { DB.Migrator().CreateTable(&models.FavoriteQuickstart{}) } + if !DB.Migrator().HasTable(&models.QuickstartProgress{}) { + DB.Migrator().CreateTable(&models.QuickstartProgress{}) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/database/db.go` around lines 89 - 105, The Init() table-creation sequence omits the QuickstartProgress model causing inconsistency with the test AutoMigrate; update the Init() function to check for and create the QuickstartProgress table just like the others by adding a HasTable/CreateTable branch for models.QuickstartProgress alongside the existing calls to DB.Migrator().HasTable and DB.Migrator().CreateTable (referencing Init, DB.Migrator(), and models.QuickstartProgress)..tekton/quickstarts-pull-request.yaml (2)
32-41: Consider using a Kubernetes Secret for PostgreSQL credentials.While these are test database credentials, hardcoding passwords in the pipeline YAML is not ideal practice. For consistency with production patterns and to avoid accidental credential exposure in logs or pipeline artifacts, consider referencing a Secret.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/quickstarts-pull-request.yaml around lines 32 - 41, Replace the hardcoded DB env values (PGSQL_USER, PGSQL_PASSWORD, PGSQL_HOSTNAME, PGSQL_PORT, PGSQL_DATABASE) with references to a Kubernetes Secret: create a Secret containing those keys (or at least the sensitive ones like PGSQL_PASSWORD and PGSQL_USER) and update the pipeline step to use valueFrom.secretKeyRef (or envFrom: secretRef) to pull values at runtime; ensure the Secret key names match the env var names used by the step so functions/steps expecting PGSQL_USER, PGSQL_PASSWORD, PGSQL_HOSTNAME, PGSQL_PORT, and PGSQL_DATABASE continue to work unchanged.
56-65: Readiness loop may not be robust enough for slow container starts.The loop waits 30 × 2s = 60 seconds, which should suffice in most cases. However, the
/dev/tcpcheck only verifies the port is open, not that PostgreSQL is ready to accept queries. Consider adding apg_isreadycheck or a simple query after the port becomes available.Enhanced readiness check
# Wait for PostgreSQL sidecar to be ready echo "Waiting for PostgreSQL..." for i in $(seq 1 30); do if bash -c "echo > /dev/tcp/$(params.PGSQL_HOSTNAME)/$(params.PGSQL_PORT)" 2>/dev/null; then - echo "PostgreSQL is ready" - break + echo "Port is open, verifying PostgreSQL accepts connections..." + if PGPASSWORD=$(params.PGSQL_PASSWORD) psql -U $(params.PGSQL_USER) -h $(params.PGSQL_HOSTNAME) -p $(params.PGSQL_PORT) -d $(params.PGSQL_DATABASE) -c "SELECT 1" >/dev/null 2>&1; then + echo "PostgreSQL is ready" + break + fi fi echo "Attempt $i/30 — waiting..." sleep 2 done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/quickstarts-pull-request.yaml around lines 56 - 65, The readiness loop that checks /dev/tcp for $(params.PGSQL_HOSTNAME)/$(params.PGSQL_PORT) should be hardened: after the port check in the for-loop, run a real PostgreSQL readiness probe (e.g., call pg_isready -h $PGSQL_HOSTNAME -p $PGSQL_PORT or execute a simple SQL like SELECT 1 via psql) with its own retry/timeout before breaking; if pg_isready/psql isn’t available fall back to an extra loop that runs a simple query against the DB and only exit when it succeeds, and consider increasing the max attempts/sleep to handle slow starts. Ensure you reference the same variables (params.PGSQL_HOSTNAME, params.PGSQL_PORT) and update the echo messages to reflect the more accurate readiness check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/database/main_test.go`:
- Line 46: The AutoMigrate call in the test setup omits the FavoriteQuickstart
model causing schema inconsistencies; update the DB.AutoMigrate invocation (the
call where DB.AutoMigrate is executed with models.Tag, models.Quickstart,
models.QuickstartProgress, models.HelpTopic) to also include
&models.FavoriteQuickstart{} so the test DB schema matches
pkg/routes/main_test.go.
---
Nitpick comments:
In @.tekton/quickstarts-pull-request.yaml:
- Around line 32-41: Replace the hardcoded DB env values (PGSQL_USER,
PGSQL_PASSWORD, PGSQL_HOSTNAME, PGSQL_PORT, PGSQL_DATABASE) with references to a
Kubernetes Secret: create a Secret containing those keys (or at least the
sensitive ones like PGSQL_PASSWORD and PGSQL_USER) and update the pipeline step
to use valueFrom.secretKeyRef (or envFrom: secretRef) to pull values at runtime;
ensure the Secret key names match the env var names used by the step so
functions/steps expecting PGSQL_USER, PGSQL_PASSWORD, PGSQL_HOSTNAME,
PGSQL_PORT, and PGSQL_DATABASE continue to work unchanged.
- Around line 56-65: The readiness loop that checks /dev/tcp for
$(params.PGSQL_HOSTNAME)/$(params.PGSQL_PORT) should be hardened: after the port
check in the for-loop, run a real PostgreSQL readiness probe (e.g., call
pg_isready -h $PGSQL_HOSTNAME -p $PGSQL_PORT or execute a simple SQL like SELECT
1 via psql) with its own retry/timeout before breaking; if pg_isready/psql isn’t
available fall back to an extra loop that runs a simple query against the DB and
only exit when it succeeds, and consider increasing the max attempts/sleep to
handle slow starts. Ensure you reference the same variables
(params.PGSQL_HOSTNAME, params.PGSQL_PORT) and update the echo messages to
reflect the more accurate readiness check.
In `@pkg/database/db.go`:
- Around line 89-105: The Init() table-creation sequence omits the
QuickstartProgress model causing inconsistency with the test AutoMigrate; update
the Init() function to check for and create the QuickstartProgress table just
like the others by adding a HasTable/CreateTable branch for
models.QuickstartProgress alongside the existing calls to DB.Migrator().HasTable
and DB.Migrator().CreateTable (referencing Init, DB.Migrator(), and
models.QuickstartProgress).
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 7f68bb13-a802-478c-9580-85e737b92c9d
📒 Files selected for processing (14)
.tekton/quickstarts-pull-request.yamlAGENTS.mdMakefilecmd/migrate/migrate.goconfig/config.godocs/testing-guidelines.mdlocal/db-compose.yamllocal/init-test-db.shpkg/database/db.gopkg/database/db_test.gopkg/database/main_test.gopkg/models/base.gopkg/models/tag.gopkg/routes/main_test.go
✅ Files skipped from review due to trivial changes (5)
- pkg/models/tag.go
- pkg/models/base.go
- cmd/migrate/migrate.go
- local/db-compose.yaml
- local/init-test-db.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- config/config.go
- Makefile
- pkg/routes/main_test.go
- docs/testing-guidelines.md
Hyperkid123
force-pushed
the
bot/RHCLOUD-42334
branch
2 times, most recently
from
71ef60d to
44481ca
Compare
Hyperkid123
force-pushed
the
bot/RHCLOUD-42334
branch
from
44481ca to
64c1273
Compare
|
@platex-rehor-bot ok this a bit too complicated. I removed your commit and created working version. The main issue was with the remote pipeline. The pipeline was not meant to be run in quickstarts, but we hoped it could be simply re-used. The issue is that the remote pipeline also spins up unleash and unelash DB. The unleash DB had some duplicate relations which prevented the gorm auto migration script form properly creating the DB schema require for quickstarts. So we need separate pipeline for quickstarts, which I have now done. The rest of changes are needed due to the infra differneces between CI and local. |
Add FavoriteQuickstart to AutoMigrate in pkg/database/main_test.go to match pkg/routes/main_test.go. Add QuickstartProgress to Init() table creation sequence alongside the other models. RHCLOUD-42334 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for rewriting the pipeline approach! That makes total sense — the shared pipeline's Unleash sidecar was causing table name conflicts with GORM auto-migration. A dedicated pipeline for quickstarts is much cleaner. I've pushed a small follow-up commit addressing two consistency issues flagged by CodeRabbit on the new code:
All SQLite tests still pass. Let me know if anything needs adjusting! |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.tekton/quickstarts-pull-request.yaml (3)
704-704: Consider pinning the PostgreSQL sidecar image tag.Using
:latestforquay.io/sclorg/postgresql-15-c9s:latestmay cause test inconsistencies if the image is updated. Consider pinning to a specific digest or version tag (e.g.,postgresql-15-c9s:c9s) for reproducible CI builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/quickstarts-pull-request.yaml at line 704, The manifest uses the floating tag quay.io/sclorg/postgresql-15-c9s:latest which can introduce CI flakiness; replace that image reference with a stable version tag or immutable digest (e.g., quay.io/sclorg/postgresql-15-c9s:c9s or quay.io/sclorg/postgresql-15-c9s@sha256:<digest>) so builds are reproducible, updating the image string in the YAML to the chosen tag/digest wherever quay.io/sclorg/postgresql-15-c9s:latest appears.
740-742: Consider whetherrunAsUser: 0is necessary.Running the unit tests step as root. If this is required for trusted artifact extraction or go-toolset operations, document the reason in a comment. Otherwise, consider using a non-root user for better security hygiene.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/quickstarts-pull-request.yaml around lines 740 - 742, The pipeline step currently sets securityContext.runAsUser: 0 which runs the unit tests as root; decide whether root is actually required for the step that contains the script: | (e.g., artifact extraction or go-toolset tasks). If root is not needed, change runAsUser to a non-root UID (or remove the setting to inherit the pod default) and if root is required, add an inline comment above securityContext or next to runAsUser explaining the specific reason (what trusted artifact or go-toolset operation requires UID 0) and any mitigation (e.g., limiting volume mounts or capabilities). Locate the securityContext block in the unit-tests step (the block containing securityContext, runAsUser, and script) to apply the change and add the explanatory comment.
32-41: Consider using a Secret for PostgreSQL credentials.The PostgreSQL credentials (
PGSQL_USER,PGSQL_PASSWORD) are hardcoded in the PipelineRun parameters. While these are only test database credentials (localhost, isolated sidecar), storing credentials in pipeline YAML is less secure than using Kubernetes Secrets. This is acceptable for now since it's an isolated test environment, but consider moving to a Secret reference for consistency with production practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/quickstarts-pull-request.yaml around lines 32 - 41, Replace the hardcoded PGSQL_USER and PGSQL_PASSWORD values with references to a Kubernetes Secret: create a Secret containing keys matching PGSQL_USER and PGSQL_PASSWORD, then update .tekton/quickstarts-pull-request.yaml to pull those values from the Secret (use secretKeyRef/valueFrom in the PipelineRun or task env where PGSQL_USER and PGSQL_PASSWORD are set); keep PGSQL_HOSTNAME, PGSQL_PORT and PGSQL_DATABASE as-is, and ensure the Tekton service account has permission to read the Secret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.tekton/quickstarts-pull-request.yaml:
- Line 704: The manifest uses the floating tag
quay.io/sclorg/postgresql-15-c9s:latest which can introduce CI flakiness;
replace that image reference with a stable version tag or immutable digest
(e.g., quay.io/sclorg/postgresql-15-c9s:c9s or
quay.io/sclorg/postgresql-15-c9s@sha256:<digest>) so builds are reproducible,
updating the image string in the YAML to the chosen tag/digest wherever
quay.io/sclorg/postgresql-15-c9s:latest appears.
- Around line 740-742: The pipeline step currently sets
securityContext.runAsUser: 0 which runs the unit tests as root; decide whether
root is actually required for the step that contains the script: | (e.g.,
artifact extraction or go-toolset tasks). If root is not needed, change
runAsUser to a non-root UID (or remove the setting to inherit the pod default)
and if root is required, add an inline comment above securityContext or next to
runAsUser explaining the specific reason (what trusted artifact or go-toolset
operation requires UID 0) and any mitigation (e.g., limiting volume mounts or
capabilities). Locate the securityContext block in the unit-tests step (the
block containing securityContext, runAsUser, and script) to apply the change and
add the explanatory comment.
- Around line 32-41: Replace the hardcoded PGSQL_USER and PGSQL_PASSWORD values
with references to a Kubernetes Secret: create a Secret containing keys matching
PGSQL_USER and PGSQL_PASSWORD, then update .tekton/quickstarts-pull-request.yaml
to pull those values from the Secret (use secretKeyRef/valueFrom in the
PipelineRun or task env where PGSQL_USER and PGSQL_PASSWORD are set); keep
PGSQL_HOSTNAME, PGSQL_PORT and PGSQL_DATABASE as-is, and ensure the Tekton
service account has permission to read the Secret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1331376e-fe24-47b9-934e-ab77cd9d17f2
📒 Files selected for processing (13)
.tekton/quickstarts-pull-request.yamlAGENTS.mdMakefileconfig/config.godocs/testing-guidelines.mdlocal/db-compose.yamllocal/init-test-db.shpkg/database/db.gopkg/database/db_test.gopkg/database/main_test.gopkg/database/testing_utils.gopkg/routes/main_test.gopkg/routes/quickstart_progress_test.go
✅ Files skipped from review due to trivial changes (3)
- local/db-compose.yaml
- local/init-test-db.sh
- pkg/database/db_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config/config.go
- Makefile
- pkg/database/main_test.go
- docs/testing-guidelines.md
RHCLOUD-42334 Pin postgresql-15-c9s sidecar to :c9s tag for reproducible CI builds. Add comment explaining why runAsUser: 0 is needed (trusted artifact extraction writes as root). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the CodeRabbit review nitpicks in 379c95c:
|
charlesmulder
left a comment
charlesmulder
left a comment
There was a problem hiding this comment.
Tested with make test (SQLite) and make test-pg (PostgreSQL). Both pass. Started local dev server, verified API endpoints work. LGTM.
3 participants
Summary
TEST_DATABASE_URLenvironment variable support so unit tests can run against PostgreSQL instead of SQLiteTEST_DATABASE_URLis set, enables full test coverage of fuzzystrmatch (Levenshtein fuzzy search), advisory locks, and other PostgreSQL-specific featuresTEST_DATABASE_URLis not set, preserving backward compatibilitymake test-pgMakefile target for convenient local PostgreSQL testingquickstarts_testdatabase alongside the existing dev databaseRHCLOUD-42334
How to test
Test plan
make testpasses with SQLite (backward compatibility)go vet ./...cleanmake test-pgpasses with local PostgreSQL (requiresmake infra)