TRT-2734: switch release consumers from BigQuery to PostgreSQL - #3736
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
cf40d96 to
fbaaf38
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughRelease metadata retrieval now uses shared PostgreSQL-backed helpers and configurable data providers. A mixed provider routes release queries to PostgreSQL and component-readiness queries to BigQuery. CLI, server, MCP, job-run, and test paths are updated accordingly. ChangesRelease provider migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServeCommand
participant newDataProvider
participant MixedProvider
participant PostgresProvider
participant PostgreSQL
participant BigQueryProvider
ServeCommand->>newDataProvider: provider configuration
newDataProvider->>MixedProvider: construct mixed provider
MixedProvider->>PostgresProvider: QueryReleases
PostgresProvider->>PostgreSQL: load release metadata
MixedProvider->>BigQueryProvider: component-readiness queries
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 18 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (18 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/sippyserver/metrics/metrics.go (1)
122-129: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRelease list should not depend on
dbcalone
RefreshMetricsDBstill runs withcrProvider/bqcenabled whendbcis nil in the component-readiness path, soreleasesstays empty and CR/disruption metrics lose release metadata. Keep the old trigger or ensure these callers never pass a nildbc.🤖 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 `@pkg/sippyserver/metrics/metrics.go` around lines 122 - 129, RefreshMetricsDB currently gates release loading only on dbc, which leaves releases empty in the component-readiness path even when crProvider or bqc are enabled. Update the release-fetching logic in RefreshMetricsDB/GetReleasesFromDB flow so release metadata is still populated for those callers, either by restoring the previous trigger condition or by guaranteeing that the readiness callers always supply a non-nil dbc; use the dbc check, crProvider, bqc, and releases symbols to locate the fix.
🧹 Nitpick comments (2)
pkg/api/releases.go (1)
503-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic string
"30"for days adjustment.
util.AdjustReleaseTime(*release.GADate, true, "30", ...)hardcodes the day-adjustment as a string that gets re-parsed internally viastrconv.ParseInt. Consider a named constant for readability, though this mirrors the existingAdjustReleaseTimesignature elsewhere.🤖 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 `@pkg/api/releases.go` at line 503, The `AdjustReleaseTime` call in `releases.go` uses a hardcoded "30" day-adjustment string, which should be replaced with a named constant for readability and maintainability. Update the `prior := util.AdjustReleaseTime(...)` usage to reference a descriptive constant defined near the release-time logic or shared with other callers, while keeping the existing `AdjustReleaseTime` signature unchanged.cmd/sippy/automatejira.go (1)
155-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDB client now created before option validation and other client setup.
dbcis now acquired at Line 155, ahead ofjiraClient(Line 164),GetJobVariants(Line 173),GetVariantJiraMap(Line 177), andf.Validate(Line 181). If any of these later steps fail, a Postgres connection was opened unnecessarily. Low impact since the process exits shortly after viareturn/panic, but worth noting as the DB client acquisition moved earlier in the flow compared to before.🤖 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 `@cmd/sippy/automatejira.go` around lines 155 - 162, The Postgres client in the automateJira flow is being opened too early in the setup sequence, before validation and the other client/map initialization steps. Move the dbc acquisition in automateJira so it happens after jiraClient, GetJobVariants, GetVariantJiraMap, and f.Validate succeed, and keep the existing error handling around GetDBClient and GetReleasesFromDB intact.
🤖 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 `@pkg/api/componentreadiness/component_report.go`:
- Around line 281-283: `NewReleaseFallbackMiddleware` is being registered even
when `c.dbc` is nil, which leaves `QueryTestDetails()` able to hit
`GetReleaseDatesFromDB()` and panic via `GetReleasesFromDB()` on `dbc.DB`.
Update the middleware setup in `component_report.go` so the `releasefallback`
path is only added when `c.dbc` is non-nil, matching the existing
`regressiontracker` guard; alternatively, ensure the multi-release analysis
branch is skipped when `dbc` is unavailable.
In `@pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go`:
- Around line 200-202: Guard the fallback DB lookup in ReleaseFallback when dbc
may be nil, since api.GetReleaseDatesFromDB and its call path into
GetReleasesFromDB dereference dbc.DB directly and can panic. Update the
ReleaseFallback middleware flow to either skip the DB-backed fallback path when
r.dbc is nil or enforce a non-nil DB in the ReleaseFallback constructor, and
make sure the handling around GetReleaseDatesFromDB uses the chosen safeguard
consistently.
In `@pkg/sippyserver/server.go`:
- Around line 903-906: The error handling in the BigQuery-backed handler uses
misleading “database” wording even though this path still calls
api.GetTestRunsAndOutputsFromBigQuery with s.bigQueryClient. Update the log and
failure response in this branch to consistently mention BigQuery instead of
database so the messages match the actual data source and are easier to debug.
---
Outside diff comments:
In `@pkg/sippyserver/metrics/metrics.go`:
- Around line 122-129: RefreshMetricsDB currently gates release loading only on
dbc, which leaves releases empty in the component-readiness path even when
crProvider or bqc are enabled. Update the release-fetching logic in
RefreshMetricsDB/GetReleasesFromDB flow so release metadata is still populated
for those callers, either by restoring the previous trigger condition or by
guaranteeing that the readiness callers always supply a non-nil dbc; use the dbc
check, crProvider, bqc, and releases symbols to locate the fix.
---
Nitpick comments:
In `@cmd/sippy/automatejira.go`:
- Around line 155-162: The Postgres client in the automateJira flow is being
opened too early in the setup sequence, before validation and the other
client/map initialization steps. Move the dbc acquisition in automateJira so it
happens after jiraClient, GetJobVariants, GetVariantJiraMap, and f.Validate
succeed, and keep the existing error handling around GetDBClient and
GetReleasesFromDB intact.
In `@pkg/api/releases.go`:
- Line 503: The `AdjustReleaseTime` call in `releases.go` uses a hardcoded "30"
day-adjustment string, which should be replaced with a named constant for
readability and maintainability. Update the `prior :=
util.AdjustReleaseTime(...)` usage to reference a descriptive constant defined
near the release-time logic or shared with other callers, while keeping the
existing `AdjustReleaseTime` signature unchanged.
🪄 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: Enterprise
Run ID: 14a3eb5d-fa70-446a-a9a6-3563023e2dc0
📒 Files selected for processing (18)
cmd/sippy/automatejira.gocmd/sippy/load.gocmd/sippy/seed_data.gopkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback_test.gopkg/api/componentreadiness/test_details.gopkg/api/job_runs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/utils.gopkg/mcp/tools/releases.gopkg/sippyserver/metrics/metrics.gopkg/sippyserver/server.go
💤 Files with no reviewable changes (6)
- pkg/api/componentreadiness/dataprovider/bigquery/releasedates.go
- pkg/api/componentreadiness/dataprovider/interface.go
- pkg/api/releases_test.go
- pkg/api/componentreadiness/dataprovider/bigquery/provider.go
- pkg/api/utils.go
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
f99e94d to
d260205
Compare
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2734 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 story 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
Bot
removed
the
do-not-merge/work-in-progress
d260205 to
afe29a0
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
/test e2e |
71baa65 to
9a59c4e
Compare
|
Scheduling required tests: |
9a59c4e to
393e724
Compare
|
/test security |
|
Scheduling required tests: |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
/lgtm |
openshift-ci
Bot
added
lgtm
Switch release data consumers to prefer PostgreSQL when available, falling back to BigQuery for deployments without a database (e.g., the QE component readiness deployment). Key changes: - getReleases() in server.go prefers PG, falls back to BQ - GetComponentReport and RefreshMetricsDB try GetReleasesFromDB first, fall back to provider.QueryReleases for BQ-only deployments - QueryReleases stays on the DataProvider interface for BQ-only deployments; PG provider delegates to GetReleasesFromDB, BQ provider converts via ReleaseRowToDefinition - Remove hardcoded releaseMetadata map from PG provider - Add GetReleaseDatesFromDB to derive CR time ranges from PG - Update releasefallback and test_details to use GetReleaseDatesFromDB - Switch automatejira, MCP releases tool, and job_runs to use PG - Delete BQ releasedates.go - Remove QueryReleaseDates from the DataProvider interface - Fix stale error messages referencing specific backends Ref: TRT-2734 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
393e724 to
deccd39
Compare
|
Had to rebase to resolve merge conflicts from #3773. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/releases.go (1)
41-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd missing
returnstatements for nil DB checks.While you are updating the error handling in these functions, please fix an existing bug: the
dbClient == nil || dbClient.DB == nilguard clauses at the top of these functions are missing areturnstatement. If the database client is nil, the functions will write an empty response but then fall through and panic whendbClient.DBis dereferenced a few lines later.
pkg/api/releases.go#L41-L42: Add areturnstatement afterRespondWithJSON(http.StatusOK, w, []struct{}{})at line 35 inPrintPullRequestsReport.pkg/api/releases.go#L337-L338: Add areturnstatement afterRespondWithJSON(http.StatusOK, w, []struct{}{})at line 333 inPrintReleasesReport.🤖 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 `@pkg/api/releases.go` around lines 41 - 42, The nil database guards in PrintPullRequestsReport and PrintReleasesReport must stop execution after sending the empty successful response. Add a return immediately after each RespondWithJSON(http.StatusOK, w, []struct{}{}) call; apply this at pkg/api/releases.go lines 41-42 and 337-338.
🤖 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.
Nitpick comments:
In `@pkg/api/releases.go`:
- Around line 41-42: The nil database guards in PrintPullRequestsReport and
PrintReleasesReport must stop execution after sending the empty successful
response. Add a return immediately after each RespondWithJSON(http.StatusOK, w,
[]struct{}{}) call; apply this at pkg/api/releases.go lines 41-42 and 337-338.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 637bc2a6-2d20-47d4-a1cb-d5335bb40b5e
📒 Files selected for processing (13)
cmd/sippy/automatejira.gocmd/sippy/component_readiness.gocmd/sippy/serve.gopkg/api/componentreadiness/dataprovider/mixed/provider.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/job_runs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/utils.gopkg/dataloader/releasedefloader/releasedefloader_test.gopkg/mcp/tools/releases.gopkg/sippyserver/server.gotest/e2e/componentreadiness/componentreadiness_test.go
💤 Files with no reviewable changes (1)
- pkg/api/utils.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/dataloader/releasedefloader/releasedefloader_test.go
- pkg/mcp/tools/releases.go
- pkg/api/componentreadiness/dataprovider/mixed/provider.go
- test/e2e/componentreadiness/componentreadiness_test.go
- pkg/api/job_runs.go
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
- cmd/sippy/automatejira.go
- pkg/sippyserver/server.go
- pkg/api/releases_test.go
- cmd/sippy/component_readiness.go
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble, neisw 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 |
|
@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. |
3 participants
Summary
Phase 2 of TRT-2734. Depends on #3679 (merged). Switches release data consumers from BigQuery to PostgreSQL using a new
MixedProviderthat routes release queries to PG and everything else to BQ.What changed
MixedProviderinpkg/api/componentreadiness/dataprovider/mixed/provider.gothat deterministically routes: release queries go to PG, everything else to BQ--data-providerflag default from"bigquery"to"default"acrossserve,automate-jira, andcomponent-readinesscommandsnewDataProvider()factory incmd/sippy/serve.gothat selects the provider based on available backends:MixedProviderBigQueryProviderPostgresProviderGetReleaseDatesFromDBto derive CR time ranges from PG GA datesgetReleases()in server.go tos.crDataProvider.QueryReleases(ctx)releaseMetadatamap from PG providerautomatejira, MCP releases tool, andjob_runsto useGetReleasesFromDBwith BQ fallbackGetReleases(),releaseGeneratorfromutils.go,TestTransformReleaseforceRefreshquery parameter parsing from/api/releases(was only used for BQ Redis cache)What stays unchanged
releasedates.go,provider.go) unchanged from mainQueryReleasesandQueryReleaseDatesremain on theDataProviderinterface for BQ-only deploymentsload.go) unchanged from main13 files changed, +435/-239
Test plan
go build ./...passesgo vet ./...passesmake lintpassesgo test ./pkg/... ./cmd/...passesmake e2epasses locally (108 tests, 0 failures)release_definitionstable is populated (36 rows in prod since Phase 1 merged)/api/releases,/api/component_readiness, and/api/component_readiness/regressionsendpointsDefinitionToRelease,ReleaseRowToDefinition, nil DB guards, andGetReleaseDatesFromDBRef: TRT-2734
Summary by CodeRabbit
--data-provideroption for release and component-readiness queries, with a newdefaultmode.