TRT-2741: Add per-request BQ/PG toggle for Component Readiness#3825
Open
mstaeble wants to merge 1 commit into
Open
TRT-2741: Add per-request BQ/PG toggle for Component Readiness#3825mstaeble wants to merge 1 commit into
mstaeble wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2741 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. |
Contributor
|
[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
Contributor
WalkthroughThe change adds request-scoped data-source selection, deterministic test-key encoding, PostgreSQL component-readiness queries, provider routing, updated report and middleware integration, frontend URL propagation, and related validation and tests. ChangesComponent Readiness data-source and PostgreSQL integration
Estimated code review effort: 5 (Critical) | ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
pkg/api/componentreadiness/dataprovider/postgres/provider.go (1)
228-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon't warn on
ErrRecordNotFound.A release with no
ReleaseDefinitionrow is an expected miss, not a failure, and this runs on every base-status query — so it will emit a warning per request for those releases. Treat not-found as a plain "no GA window" result and reserve the warning for real query errors.♻️ Suggested change
if err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return false + } log.WithError(err).WithField("release", release). Warn("failed to query GA date, falling back to prefix-sum query") return false }🤖 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/componentreadiness/dataprovider/postgres/provider.go` around lines 228 - 241, Update PostgresProvider.baseMatchesGAWindow to detect the database not-found error and return false without logging; keep the existing warning for other query errors, preserving the fallback behavior for all failures.pkg/api/componentreadiness/dataprovider/postgres/variants.go (1)
102-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the group-ID allocation logic.
buildVariantGroupMappingandbuildColumnGroupMappingare pure functions with non-trivial invariants: sequential group IDs, synthetic column group IDs starting atlen(groupToVariants)(which only holds because the real IDs are dense0..N-1), and the "call once per map" contract. These are exactly the kind of assumptions that silently break later and corrupt grouped results. Table-driven tests here need no database.Also note
groupToVariants[nextGroupID] = dimsstores the same map reference held byvariantLookup; if any future caller mutates one, the other changes too. A shallow copy would remove that coupling.As per coding guidelines, "new Go functions and methods need unit tests".
Also applies to: 160-203
🤖 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/componentreadiness/dataprovider/postgres/variants.go` around lines 102 - 146, Add table-driven unit tests for buildVariantGroupMapping and buildColumnGroupMapping, covering sequential dense group IDs, synthetic column IDs beginning at len(groupToVariants), duplicate grouping, and the requirement that each mapping is built once per input map. Also copy variant dimension maps when assigning groupToVariants so returned mappings do not share mutable references with variantLookup.Source: Coding guidelines
pkg/api/componentreadiness/dataprovider/mixed/provider.go (1)
36-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCentralize Component Readiness data-source constants and make mixed routing fail-safe.
The request docs at
pkg/apis/api/componentreport/reqopts/types.golist"postgres"/"bigquery", while routing here and atpkg/api/componentreadiness/test_details.go:440still uses magic literals. Also,NewMixedProvideralways instantiates apostgres.PostgresProvider; route PostgreSQL test queries only whendbcis available (p.pg != nil) or make the constructor/newDataProviderreturn an error otherwise so PostgreSQL routes do not dereference a nil DB client.🤖 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/componentreadiness/dataprovider/mixed/provider.go` around lines 36 - 41, Centralize the “postgres” and “bigquery” values in the request-options constants and reuse them in MixedProvider.providerFor and the routing logic in test_details.go instead of magic literals. Make NewMixedProvider/newDataProvider fail safely when no PostgreSQL DB client is available: either skip PostgreSQL routing when p.pg is nil or return an error during construction, ensuring no PostgreSQL query dereferences a nil client.pkg/api/componentreadiness/dataprovider/postgres/cr_queries.go (2)
70-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
status = 12literal.A bare status code is opaque here; a short "why" comment (or a shared constant used elsewhere for failure status) helps future readers.
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 `@pkg/api/componentreadiness/dataprovider/postgres/cr_queries.go` around lines 70 - 83, Document the meaning of the status = 12 predicate in the lastFailureLateral query by reusing an existing shared failure-status constant if available, or adding a brief nearby comment explaining why this status identifies failed runs. Keep the query behavior unchanged and avoid unrelated documentation.Source: Coding guidelines
350-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the three
SET LOCALstatements into oneExec.Three round trips with an identical error message add noise without value.
♻️ Suggested consolidation
- if err := tx.Exec("SET LOCAL max_parallel_workers_per_gather = 4").Error; err != nil { - return fmt.Errorf("setting parallel query hints: %w", err) - } - if err := tx.Exec("SET LOCAL parallel_setup_cost = 0").Error; err != nil { - return fmt.Errorf("setting parallel query hints: %w", err) - } - if err := tx.Exec("SET LOCAL parallel_tuple_cost = 0").Error; err != nil { - return fmt.Errorf("setting parallel query hints: %w", err) - } + // Encourage parallel plans for these wide aggregations. + hints := "SET LOCAL max_parallel_workers_per_gather = 4;" + + "SET LOCAL parallel_setup_cost = 0;" + + "SET LOCAL parallel_tuple_cost = 0" + if err := tx.Exec(hints).Error; err != nil { + return fmt.Errorf("setting parallel query hints: %w", err) + }🤖 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/componentreadiness/dataprovider/postgres/cr_queries.go` around lines 350 - 358, In the transaction setup flow containing the three parallel-query hint statements, combine the `SET LOCAL max_parallel_workers_per_gather`, `parallel_setup_cost`, and `parallel_tuple_cost` commands into a single `tx.Exec` call. Keep one error check returning the existing “setting parallel query hints” wrapped error.pkg/sippyserver/server.go (1)
1021-1022: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated variant-lookup boilerplate.
The same three lines (
reqopts.RequestOptions{DataSource: param.SafeRead(req, "dataSource")}+GetJobVariants+ error aggregation) now appear in three handlers. A small helper such ass.jobVariantsForRequest(req) (crtest.JobVariants, error)would keep the data-source plumbing in one place as more callers appear.Also applies to: 1094-1095, 1147-1148
🤖 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/server.go` around lines 1021 - 1022, The repeated job-variant lookup and error aggregation in the three handlers should be centralized. Add a server helper such as jobVariantsForRequest using the request context, crDataProvider, and dataSource extracted via param.SafeRead, then update the affected handlers to call it while preserving their existing error handling and outputs.pkg/api/componentreadiness/test_details.go (2)
439-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new postgres-backfill explanation branch.
internalGenerateTestDetailsReportnow emits a distinct user-facing explanation whenbaseStatusis empty and the data source is postgres, but no test in this batch exercises that branch.As per path instructions,
**/*.{go,tsx,jsx}: "New or modified functionality should include test coverage: new Go functions and methods need unit tests, bug fixes need regression tests."🤖 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/componentreadiness/test_details.go` around lines 439 - 446, The new PostgreSQL backfill explanation branch in internalGenerateTestDetailsReport lacks test coverage. Add a unit test exercising empty baseStatus with c.ReqOptions.DataSource set to "postgres", and assert the resulting TestComparison.Explanations contains the expected user-facing explanation; retain coverage for other data sources or non-empty baseStatus behavior.
439-446: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winShared constants for
reqoptsDataSource values
DataSourceis set from a user request value inpkg/sippyserver/server.go, so routing/comparison points should derive from one shared set instead of string literals. Add exported constants for"bigquery"and"postgres"inpkg/apis/api/componentreport/reqoptsand use the non-default provider constant inpkg/api/componentreadiness/dataprovider/mixed/provider.goand similar decision points.🤖 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/componentreadiness/test_details.go` around lines 439 - 446, Add exported DataSource constants for the “bigquery” and “postgres” values in the reqopts package, then replace raw DataSource string literals in test-details and other routing/comparison points, including the non-default provider logic in mixed provider.go, with the shared constants. Preserve the existing provider-selection and explanation behavior.
🤖 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/dataprovider/bigquery/querygenerators.go`:
- Around line 849-851: Version the cache namespaces in querygenerators.go to
invalidate entries created with the old identity format: bump both
BaseTestStatus~ and SampleTestStatus~ prefixes near the status encoding flow,
and bump both BaseJobRunTestStatusV2~ and SampleJobRunTestStatusV2~ prefixes at
line 1130. Keep the existing cache key structure unchanged apart from the
namespace versions.
In `@pkg/apis/api/componentreport/crtest/types.go`:
- Around line 105-113: Update VariantListToMap and VariantListToMapWithWarnings
in pkg/api/utils.go to split each variant at only the first colon using
strings.Cut, preserving colons in values and round-tripping inputs such as
Label:a:b; add a regression test covering URL generation followed by parsing of
such a value, while preserving consistent HATEOAS client behavior.
---
Nitpick comments:
In `@pkg/api/componentreadiness/dataprovider/mixed/provider.go`:
- Around line 36-41: Centralize the “postgres” and “bigquery” values in the
request-options constants and reuse them in MixedProvider.providerFor and the
routing logic in test_details.go instead of magic literals. Make
NewMixedProvider/newDataProvider fail safely when no PostgreSQL DB client is
available: either skip PostgreSQL routing when p.pg is nil or return an error
during construction, ensuring no PostgreSQL query dereferences a nil client.
In `@pkg/api/componentreadiness/dataprovider/postgres/cr_queries.go`:
- Around line 70-83: Document the meaning of the status = 12 predicate in the
lastFailureLateral query by reusing an existing shared failure-status constant
if available, or adding a brief nearby comment explaining why this status
identifies failed runs. Keep the query behavior unchanged and avoid unrelated
documentation.
- Around line 350-358: In the transaction setup flow containing the three
parallel-query hint statements, combine the `SET LOCAL
max_parallel_workers_per_gather`, `parallel_setup_cost`, and
`parallel_tuple_cost` commands into a single `tx.Exec` call. Keep one error
check returning the existing “setting parallel query hints” wrapped error.
In `@pkg/api/componentreadiness/dataprovider/postgres/provider.go`:
- Around line 228-241: Update PostgresProvider.baseMatchesGAWindow to detect the
database not-found error and return false without logging; keep the existing
warning for other query errors, preserving the fallback behavior for all
failures.
In `@pkg/api/componentreadiness/dataprovider/postgres/variants.go`:
- Around line 102-146: Add table-driven unit tests for buildVariantGroupMapping
and buildColumnGroupMapping, covering sequential dense group IDs, synthetic
column IDs beginning at len(groupToVariants), duplicate grouping, and the
requirement that each mapping is built once per input map. Also copy variant
dimension maps when assigning groupToVariants so returned mappings do not share
mutable references with variantLookup.
In `@pkg/api/componentreadiness/test_details.go`:
- Around line 439-446: The new PostgreSQL backfill explanation branch in
internalGenerateTestDetailsReport lacks test coverage. Add a unit test
exercising empty baseStatus with c.ReqOptions.DataSource set to "postgres", and
assert the resulting TestComparison.Explanations contains the expected
user-facing explanation; retain coverage for other data sources or non-empty
baseStatus behavior.
- Around line 439-446: Add exported DataSource constants for the “bigquery” and
“postgres” values in the reqopts package, then replace raw DataSource string
literals in test-details and other routing/comparison points, including the
non-default provider logic in mixed provider.go, with the shared constants.
Preserve the existing provider-selection and explanation behavior.
In `@pkg/sippyserver/server.go`:
- Around line 1021-1022: The repeated job-variant lookup and error aggregation
in the three handlers should be centralized. Add a server helper such as
jobVariantsForRequest using the request context, crDataProvider, and dataSource
extracted via param.SafeRead, then update the affected handlers to call it while
preserving their existing error handling and outputs.
🪄 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: e5867338-99ed-4e6f-bd14-a60e33f11da2
📒 Files selected for processing (40)
cmd/sippy/annotatejobruns.gocmd/sippy/automatejira.gopkg/api/cache.gopkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/component_report_test.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/bigquery/querygenerators.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/mixed/provider.gopkg/api/componentreadiness/dataprovider/postgres/cr_queries.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/dataprovider/postgres/variants.gopkg/api/componentreadiness/middleware/linkinjector/linkinjector.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback_test.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/regressiontracker.gopkg/api/componentreadiness/test_details.gopkg/api/componentreadiness/triage.gopkg/api/componentreadiness/utils/queryparamparser.gopkg/api/componentreadiness/utils/utils.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/utils.gopkg/apis/api/componentreport/crstatus/types.gopkg/apis/api/componentreport/crtest/types.gopkg/apis/api/componentreport/crtest/types_test.gopkg/apis/api/componentreport/reqopts/types.gopkg/dataloader/gateststatus/loader.gopkg/db/models/prow.gopkg/db/query/cumulative_query.gopkg/db/query/feature_gates.gopkg/sippyserver/server.gopkg/util/param/param.gosippy-ng/src/component_readiness/CompReadyUtils.jsxsippy-ng/src/component_readiness/CompReadyVars.jsxsippy-ng/src/component_readiness/ComponentReadiness.jsxsippy-ng/src/component_readiness/ComponentReadinessIndicator.jsxsippy-ng/src/component_readiness/ReleaseSelector.jsx
mstaeble
force-pushed
the
trt-2741-phase3
branch
3 times, most recently
from
July 25, 2026 14:48
b9d39bb to
b8c7a98
Compare
mstaeble
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/database-tuning.md`:
- Around line 8-11: Update the random_page_cost guidance in the document to
explicitly state that the application’s per-session setting overrides the
server-level RDS parameter-group value, and identify the required configuration
operators must change to affect Sippy’s effective value. Ensure the server-level
section is described as applicable only when no session override is set.
In `@pkg/api/componentreadiness/dataprovider/postgres/provider_test.go`:
- Around line 59-98: Update filterByDBGroupBy and its TestFilterByDBGroupBy
cases to accept sets.Set[string] for dbGroupBy instead of map[string]bool.
Remove the conversion from VariantOption.DBGroupBy in provider.go and pass the
set directly, updating test fixtures and assertions to use the existing set API.
In `@pkg/api/componentreadiness/dataprovider/postgres/variants_test.go`:
- Around line 118-127: Strengthen the tests around buildVariantGroupMapping by
asserting the exact valuesClause string and complete groupToVariants contents
for both collapsed and distinct projection cases, not just emptiness and counts.
Add expected mapping data to the relevant test cases and compare the generated
entries against it while preserving the existing validation.
🪄 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: f23b3b2d-5af5-40fc-a7a5-8f3500b85056
📒 Files selected for processing (44)
README.mdcmd/sippy/annotatejobruns.gocmd/sippy/automatejira.godocs/database-tuning.mdpkg/api/cache.gopkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/component_report_test.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/bigquery/querygenerators.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/mixed/provider.gopkg/api/componentreadiness/dataprovider/postgres/cr_queries.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/dataprovider/postgres/provider_test.gopkg/api/componentreadiness/dataprovider/postgres/variants.gopkg/api/componentreadiness/dataprovider/postgres/variants_test.gopkg/api/componentreadiness/middleware/linkinjector/linkinjector.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback_test.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/regressiontracker.gopkg/api/componentreadiness/test_details.gopkg/api/componentreadiness/triage.gopkg/api/componentreadiness/utils/queryparamparser.gopkg/api/componentreadiness/utils/utils.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/utils.gopkg/apis/api/componentreport/crstatus/types.gopkg/apis/api/componentreport/crtest/types.gopkg/apis/api/componentreport/crtest/types_test.gopkg/apis/api/componentreport/reqopts/types.gopkg/dataloader/gateststatus/loader.gopkg/db/models/prow.gopkg/db/query/cumulative_query.gopkg/db/query/feature_gates.gopkg/sippyserver/server.gopkg/util/param/param.gosippy-ng/src/component_readiness/CompReadyUtils.jsxsippy-ng/src/component_readiness/CompReadyVars.jsxsippy-ng/src/component_readiness/ComponentReadiness.jsxsippy-ng/src/component_readiness/ComponentReadinessIndicator.jsxsippy-ng/src/component_readiness/ReleaseSelector.jsx
🚧 Files skipped from review as they are similar to previous changes (37)
- pkg/db/query/feature_gates.go
- pkg/db/models/prow.go
- pkg/apis/api/componentreport/reqopts/types.go
- pkg/api/componentreadiness/queryparamparser_test.go
- sippy-ng/src/component_readiness/ComponentReadiness.jsx
- pkg/api/componentreadiness/utils/queryparamparser.go
- pkg/api/componentreadiness/triage.go
- pkg/api/componentreadiness/regressiontracker.go
- pkg/api/componentreadiness/middleware/linkinjector/linkinjector.go
- sippy-ng/src/component_readiness/ComponentReadinessIndicator.jsx
- pkg/api/utils.go
- pkg/dataloader/gateststatus/loader.go
- cmd/sippy/annotatejobruns.go
- cmd/sippy/automatejira.go
- pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go
- sippy-ng/src/component_readiness/ReleaseSelector.jsx
- pkg/db/query/cumulative_query.go
- pkg/util/param/param.go
- pkg/api/componentreadiness/middleware/releasefallback/releasefallback_test.go
- pkg/apis/api/componentreport/crstatus/types.go
- pkg/api/componentreadiness/test_details.go
- pkg/api/cache.go
- sippy-ng/src/component_readiness/CompReadyUtils.jsx
- pkg/api/componentreadiness/dataprovider/interface.go
- pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.go
- pkg/api/componentreadiness/dataprovider/postgres/variants.go
- pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.go
- pkg/api/componentreadiness/dataprovider/postgres/cr_queries.go
- pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go
- pkg/api/componentreadiness/utils/utils.go
- sippy-ng/src/component_readiness/CompReadyVars.jsx
- pkg/sippyserver/server.go
- pkg/apis/api/componentreport/crtest/types.go
- pkg/api/componentreadiness/utils/utils_test.go
- pkg/api/componentreadiness/dataprovider/mixed/provider.go
- pkg/api/componentreadiness/component_report_test.go
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
Adds a `dataSource` query parameter to the Component Readiness API that allows switching between BigQuery and PostgreSQL on a per-request basis. The frontend exposes this as a toggle in the release selector. PostgreSQL query optimizations: - Restructure placeholder queries to group at the component level, reducing intermediate rows from 1.1M to a few thousand - Fix GA date query to use First instead of GORM Pluck (which couldn't scan a PostgreSQL date column into *time.Time) - Remove enable_nestloop=off which was catastrophic for LATERAL joins - Reserve parallel worker hints (max_parallel_workers_per_gather=4, parallel_setup_cost=0, parallel_tuple_cost=0) for failure queries only, since placeholder queries perform well without them - Use MATERIALIZED CTE in test_details to force test_id-driven plan - Extract row scanning into shared scanRows helper Also fixes variant validation to return HTTP 400 (ValidationError) instead of 500 for invalid variant parameters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mstaeble
force-pushed
the
trt-2741-phase3
branch
from
July 25, 2026 16:44
b8c7a98 to
0faeda5
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/sippyserver/server.go (2)
1097-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the underlying variant-query errors.
Formatting
errswith%vdiscards error unwrapping. Join the returned errors and wrap with%wso callers can retainerrors.Is/errors.Asbehavior. As per coding guidelines, wrap errors with context usingfmt.Errorfand%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 `@pkg/sippyserver/server.go` around lines 1097 - 1099, Update the error return in the variant-query path to join errs and wrap the joined error with fmt.Errorf using %w, preserving the existing “failed to get job variants” context while retaining errors.Is/errors.As behavior.Source: Coding guidelines
1095-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid parsing
dataSourcetwice.Line 1095 calls
param.SafeRead(req, "dataSource"), andutils.ParseComponentReportRequestrepeats that call at Line 1106. Pass the validated value into the parser (or split parsing) so invalid input is not validated and logged twice. As per path instructions, “Avoid calling the same utility function multiple times with identical arguments in the same code path.”🤖 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/server.go` around lines 1095 - 1096, Update the request handling around GetJobVariants and utils.ParseComponentReportRequest to read and validate dataSource once, then pass the validated value into the parser or split parsing accordingly. Remove the duplicate param.SafeRead call while preserving existing validation and logging behavior for invalid input.Source: Path instructions
🤖 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/sippyserver/server.go`:
- Around line 1097-1099: Update the error return in the variant-query path to
join errs and wrap the joined error with fmt.Errorf using %w, preserving the
existing “failed to get job variants” context while retaining
errors.Is/errors.As behavior.
- Around line 1095-1096: Update the request handling around GetJobVariants and
utils.ParseComponentReportRequest to read and validate dataSource once, then
pass the validated value into the parser or split parsing accordingly. Remove
the duplicate param.SafeRead call while preserving existing validation and
logging behavior for invalid input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 948b5f09-6e96-413a-98da-141e0d6e4b85
📒 Files selected for processing (44)
README.mdcmd/sippy/annotatejobruns.gocmd/sippy/automatejira.godocs/database-tuning.mdpkg/api/cache.gopkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/component_report_test.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/bigquery/querygenerators.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/mixed/provider.gopkg/api/componentreadiness/dataprovider/postgres/cr_queries.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/dataprovider/postgres/provider_test.gopkg/api/componentreadiness/dataprovider/postgres/variants.gopkg/api/componentreadiness/dataprovider/postgres/variants_test.gopkg/api/componentreadiness/middleware/linkinjector/linkinjector.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback_test.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/regressiontracker.gopkg/api/componentreadiness/test_details.gopkg/api/componentreadiness/triage.gopkg/api/componentreadiness/utils/queryparamparser.gopkg/api/componentreadiness/utils/utils.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/utils.gopkg/apis/api/componentreport/crstatus/types.gopkg/apis/api/componentreport/crtest/types.gopkg/apis/api/componentreport/crtest/types_test.gopkg/apis/api/componentreport/reqopts/types.gopkg/dataloader/gateststatus/loader.gopkg/db/models/prow.gopkg/db/query/cumulative_query.gopkg/db/query/feature_gates.gopkg/sippyserver/server.gopkg/util/param/param.gosippy-ng/src/component_readiness/CompReadyUtils.jsxsippy-ng/src/component_readiness/CompReadyVars.jsxsippy-ng/src/component_readiness/ComponentReadiness.jsxsippy-ng/src/component_readiness/ComponentReadinessIndicator.jsxsippy-ng/src/component_readiness/ReleaseSelector.jsx
🚧 Files skipped from review as they are similar to previous changes (37)
- pkg/api/componentreadiness/queryparamparser_test.go
- pkg/db/query/feature_gates.go
- cmd/sippy/automatejira.go
- sippy-ng/src/component_readiness/ComponentReadinessIndicator.jsx
- README.md
- pkg/api/componentreadiness/utils/queryparamparser.go
- pkg/api/componentreadiness/middleware/linkinjector/linkinjector.go
- docs/database-tuning.md
- pkg/util/param/param.go
- pkg/api/componentreadiness/dataprovider/postgres/provider_test.go
- pkg/apis/api/componentreport/crtest/types_test.go
- pkg/apis/api/componentreport/reqopts/types.go
- pkg/api/componentreadiness/dataprovider/postgres/variants_test.go
- pkg/api/utils.go
- pkg/api/componentreadiness/triage.go
- pkg/api/componentreadiness/dataprovider/interface.go
- sippy-ng/src/component_readiness/CompReadyUtils.jsx
- pkg/db/query/cumulative_query.go
- pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go
- pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go
- pkg/api/componentreadiness/middleware/releasefallback/releasefallback_test.go
- pkg/apis/api/componentreport/crstatus/types.go
- pkg/db/models/prow.go
- pkg/api/componentreadiness/regressiontracker.go
- pkg/api/componentreadiness/dataprovider/postgres/variants.go
- pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.go
- sippy-ng/src/component_readiness/CompReadyVars.jsx
- pkg/apis/api/componentreport/crtest/types.go
- pkg/api/componentreadiness/dataprovider/postgres/cr_queries.go
- pkg/api/componentreadiness/utils/utils.go
- pkg/api/componentreadiness/dataprovider/bigquery/provider.go
- pkg/api/cache.go
- pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.go
- pkg/api/componentreadiness/component_report_test.go
- pkg/api/componentreadiness/component_report.go
- pkg/api/componentreadiness/utils/utils_test.go
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
changed the title
mstaeble
marked this pull request as ready for review
July 26, 2026 12:32
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
Scheduling required tests: |
Contributor
Author
|
/test e2e |
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dataSourcequery parameter and UI toggle (cookie-based) to switch Component Readiness between BigQuery and PostgreSQL data providers per requesttest_cumulative_summaries, with SQL-level variant grouping via VALUES clause joinsprow_ga_raw_test_datawhen the base date range matches a pre-computed GA window (1, 30, or 90 days)work_mem=128MBsettingTestStatus.Variantsfrom[]stringtomap[string]stringthroughout, with cache unmarshal fallback for the type transitionKeyWithVariants.Encode()/DecodeColumnID()using null-byte-separated encoding for deterministic, collision-free map keysFindOpenRegressionlookups by testID for O(1) access instead of linear scanCompareVariantsintoIncludeVariantsfor sample queries in cross-compare viewsDeserializeTestKeytoIdentificationFromStatusto reflect actual behaviorKnown parity gaps (BQ vs PG)
These are documented and accepted for this phase:
last_failureglobally per (test_id, suite_id) across all variants. BQ scopes it per variant group within its GROUP BY. Impact is cosmetic only (display field, not used for regression logic). Fixing requires 3 additional joins inside the LATERAL subquery.prow_job_runsto exclude InfraFailure-labeled runs. BQ excludes them. Same cosmetic impact and same fix path as above.lifecycle IN ('blocking')per-test in sample queries. PG has no lifecycle column and includes informing-lifecycle tests. Tracked separately.config/openshift.yaml.Test plan
go vet ./pkg/api/componentreadiness/...passesmake lintpassesmake testpasses (Go + Vitest)make e2epassesdataSource=postgresreturns results for component_readiness and test_details endpointsdataSource=postgresresponse times are acceptable (<5s for component_readiness, <1s for test_details)🤖 Generated with Claude Code
Summary by CodeRabbit
dataSourcesupport to select BigQuery or PostgreSQL for Component Readiness reports, test details, and job-variant lookups.