TRT-2814: Eliminate prow_job_runs_report_matview - #3828
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. |
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2814 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. |
|
[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
|
Tip For best results, initiate chat on the files or code changes.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThe job-runs report now uses dynamic queries over base tables instead of a materialized view, supports expanded filter semantics, and concurrently enriches paginated rows with test names, pull requests, and annotations. Integration tests cover filtering, sorting, pagination, enrichment, and boundary cases. ChangesJob runs reporting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JobsRunsReportFromDB
participant PostgreSQL
participant errgroup
JobsRunsReportFromDB->>PostgreSQL: Execute dynamic filtered and paginated query
PostgreSQL-->>JobsRunsReportFromDB: Return base job-run rows and total count
JobsRunsReportFromDB->>errgroup: Start concurrent enrichment tasks
errgroup->>PostgreSQL: Query test names, pull requests, and annotations
PostgreSQL-->>errgroup: Return enrichment data
errgroup-->>JobsRunsReportFromDB: Complete enriched report rows
Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 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: 4
🧹 Nitpick comments (4)
pkg/api/job_runs.go (2)
151-175: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
errgroup.WithContextand add context to enrichment errors.Four independent queries fan out per request with no cancellation and no error context. When one fails, the others keep running to completion against the pool, and the caller receives a bare driver error with no indication of which enrichment failed.
errgroup.WithContextplus a shortfmt.Errorf("...: %w", err)per branch (or per helper) makes failures diagnosable and lets siblings abort early.🤖 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/job_runs.go` around lines 151 - 175, The enrichment fan-out around the errgroup in the job-run loading flow lacks cancellation and identifies no failing enrichment. Replace the plain errgroup with errgroup.WithContext, pass the derived context into each enrichment helper if supported, and wrap each branch’s error with a short enrichment-specific fmt.Errorf using %w so sibling queries cancel when one fails.
204-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the new pure helpers.
analyzeJobRunFilters,isTestNameField, andtestNameFilterSQLare dependency-free and encode the join/dispatch decisions the whole refactor rests on;pkg/api/job_runs_test.goalready exists. Table-driven cases (sort ontest_flakes, sort on a PR column, mixed filter fields, each test-name operator) would lock in behavior without a database.As per coding guidelines, "new Go functions and methods need unit 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/job_runs.go` around lines 204 - 233, Add table-driven unit tests in the existing pkg/api/job_runs_test.go for analyzeJobRunFilters, isTestNameField, and testNameFilterSQL. Cover sorting by test_flakes, sorting by a pull-request column, mixed filter fields, and every supported test-name operator, asserting the expected helper outputs without database dependencies.Source: Coding guidelines
pkg/filter/filterable.go (2)
148-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnknown operator silently produces invalid SQL.
The
defaultbranch emitsUnknownFilterOperator()as a SQL fragment. Callers (e.g.applyJobRunFiltersinpkg/api/job_runs.go) splice this straight into theWHEREclause, so a badoperatorValuefrom the client surfaces as a Postgres "function does not exist" error / 500 rather than a 400 validation error. Consider returning anerror(or an empty fragment the caller can reject) so the API can respond withValidationError.Note this is a behavior widening compared to
andFilterToSQL, which simply ignores unknown operators.🤖 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/filter/filterable.go` around lines 148 - 151, The default branch in the filter-to-SQL conversion must reject unknown operators instead of returning the invalid “UnknownFilterOperator()” SQL fragment. Update the relevant filter conversion method and its callers, including applyJobRunFilters, to propagate the validation error so invalid client operators produce a ValidationError/400 response; preserve handling for supported operators and the existing andFilterToSQL behavior.
154-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for the new exported SQL generators.
FilterItemToSQLandFilterFieldToSQLnow carry the negation/array/timestamp semantics thatorFilterToSQLused to own, and they are consumed cross-package bypkg/api/job_runs.go.pkg/filter/filterable_test.gois the natural home for table-driven assertions on the emitted fragment + param per operator (especiallyOperatorHasEntrywithNot, andOperatorIsNotEmpty, where the oldoptNot(!f.Not)inversion was replaced byWrapNot).As per coding guidelines, "new Go functions and methods need unit 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/filter/filterable.go` around lines 154 - 176, Add table-driven unit tests in filterable_test.go covering the exported FilterItemToSQL and FilterFieldToSQL methods, asserting both emitted SQL fragments and parameters for each operator. Include array-aware negation for OperatorHasEntry with Not, timestamp handling in FilterFieldToSQL, and the OperatorIsNotEmpty behavior using WrapNot rather than the former optNot inversion. Reuse existing test conventions and cover representative scalar, array, and timestamp cases.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 `@pkg/api/job_runs.go`:
- Around line 318-352: Reject unsupported operators instead of converting them
into SQL. In pkg/api/job_runs.go lines 318-352, update testNameFilterSQL to
handle only supported operators explicitly and return an error for unknown
values, propagating it to the existing 400 ValidationError path. In
pkg/filter/filterable.go lines 148-151, change FilterItemToSQL to return an
error or empty fragment with an ok indicator rather than
UnknownFilterOperator(), and update its callers to reject invalid operators as
client validation errors.
- Around line 305-311: Update the clause construction in the job-run query
around the filter.LinkOperatorOr branch so OR-joined clauses are explicitly
grouped in parentheses before passing them to q.Where. Preserve the existing AND
join behavior and argument ordering, ensuring subsequent release and timestamp
conditions cannot change the OR expression’s precedence.
- Around line 386-421: Update enrichJobRunsWithTestNames and its query to
populate the ran_test_names API field with all test names associated with each
job run, while retaining the existing failed and flaked name projections for
statuses 12 and 13. Ensure job runs with tests outside those statuses receive
their names so frontend results do not default to Pass.
In `@sippy-ng/src/jobs/JobRunsTable.jsx`:
- Around line 365-381: Update the sortField initialization or validation in the
JobRunsTable component so URL-provided fields that map to columns with sortable:
false fall back to the existing default sort field. Preserve valid sortable URL
values and use the column definitions as the source of truth, including
flaked_test_names and ran_test_names.
---
Nitpick comments:
In `@pkg/api/job_runs.go`:
- Around line 151-175: The enrichment fan-out around the errgroup in the job-run
loading flow lacks cancellation and identifies no failing enrichment. Replace
the plain errgroup with errgroup.WithContext, pass the derived context into each
enrichment helper if supported, and wrap each branch’s error with a short
enrichment-specific fmt.Errorf using %w so sibling queries cancel when one
fails.
- Around line 204-233: Add table-driven unit tests in the existing
pkg/api/job_runs_test.go for analyzeJobRunFilters, isTestNameField, and
testNameFilterSQL. Cover sorting by test_flakes, sorting by a pull-request
column, mixed filter fields, and every supported test-name operator, asserting
the expected helper outputs without database dependencies.
In `@pkg/filter/filterable.go`:
- Around line 148-151: The default branch in the filter-to-SQL conversion must
reject unknown operators instead of returning the invalid
“UnknownFilterOperator()” SQL fragment. Update the relevant filter conversion
method and its callers, including applyJobRunFilters, to propagate the
validation error so invalid client operators produce a ValidationError/400
response; preserve handling for supported operators and the existing
andFilterToSQL behavior.
- Around line 154-176: Add table-driven unit tests in filterable_test.go
covering the exported FilterItemToSQL and FilterFieldToSQL methods, asserting
both emitted SQL fragments and parameters for each operator. Include array-aware
negation for OperatorHasEntry with Not, timestamp handling in FilterFieldToSQL,
and the OperatorIsNotEmpty behavior using WrapNot rather than the former optNot
inversion. Reuse existing test conventions and cover representative scalar,
array, and timestamp cases.
🪄 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: 87c0d531-857b-4583-a19c-e8b85ce1e8ae
📒 Files selected for processing (5)
pkg/api/job_runs.gopkg/db/views.gopkg/filter/filterable.gopkg/flags/postgres_benchmarking_test.gosippy-ng/src/jobs/JobRunsTable.jsx
💤 Files with no reviewable changes (2)
- pkg/db/views.go
- pkg/flags/postgres_benchmarking_test.go
59c83ec to
d771bb9
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
@coderabbitai resolve |
openshift-ci
Bot
added
the
ready-for-human-review
✅ Action performedComments resolved and changes approved. |
mstaeble
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
9354237 to
e1fb97c
Compare
openshift-ci
Bot
added
the
needs-rebase
e1fb97c to
42d55af
Compare
openshift-ci
Bot
removed
the
needs-rebase
42d55af to
11b0f66
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/integration/job_runs_report_test.go (1)
22-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefix these package-level vars like the
jr*helpers.
reportEndandlookbackare generic names in the sharedintegrationpackage; the file already adopts ajrprefix for helpers to avoid collisions withcomponent_readiness_test.go(Line 155). Same treatment here avoids a future redeclaration conflict.🤖 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/job_runs_report_test.go` around lines 22 - 25, Rename the package-level variables reportEnd and lookback to use the existing jr-prefixed naming convention, and update all references in the test accordingly. Preserve their current time values and behavior while avoiding collisions with other integration tests.pkg/api/job_runs.go (1)
213-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
v1.TestStatus*constants instead of literal 12/13.The same codes are hardcoded again in
enrichJobRunsWithTestNames(Lines 329-334), so the mapping is duplicated in three places. Deriving them fromv1.TestStatusFailure/v1.TestStatusFlakekeeps the report in sync if the enum ever changes.♻️ Suggested change
var testNameStatuses = map[string]int{ - "ran_test_names": 0, - "failed_test_names": 12, - "flaked_test_names": 13, + "ran_test_names": 0, + "failed_test_names": int(v1.TestStatusFailure), + "flaked_test_names": int(v1.TestStatusFlake), }🤖 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/job_runs.go` around lines 213 - 221, The testNameStatuses map uses hardcoded status codes that should come from the v1 enum constants. Replace the 12 and 13 values with v1.TestStatusFailure and v1.TestStatusFlake, and update the corresponding mappings in enrichJobRunsWithTestNames to reuse these constants rather than duplicating literals.
🤖 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/job_runs.go`:
- Around line 368-379: The PR-resolution queries use nondeterministic DISTINCT
ON selection. In pkg/api/job_runs.go lines 368-379, extend the ORDER BY in the
Raw query with the same unique secondary ordering, such as
jrpp.prow_pull_request_id DESC; apply that identical ordering in the LEFT JOIN
DISTINCT ON subquery at lines 73-77 so both paths consistently select the same
PR.
- Around line 88-101: Update the job-runs query construction around the existing
count and join logic to always apply a deterministic order before LIMIT/OFFSET
pagination, including when no SortField is provided. Preserve the requested sort
direction and append the unique prow_job_runs.id column as a final tiebreaker
for every supported sort field, including timestamp, test_failures, and job.
---
Nitpick comments:
In `@pkg/api/job_runs.go`:
- Around line 213-221: The testNameStatuses map uses hardcoded status codes that
should come from the v1 enum constants. Replace the 12 and 13 values with
v1.TestStatusFailure and v1.TestStatusFlake, and update the corresponding
mappings in enrichJobRunsWithTestNames to reuse these constants rather than
duplicating literals.
In `@test/integration/job_runs_report_test.go`:
- Around line 22-25: Rename the package-level variables reportEnd and lookback
to use the existing jr-prefixed naming convention, and update all references in
the test accordingly. Preserve their current time values and behavior while
avoiding collisions with other integration 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e702e4b6-2cef-47c8-9aaa-22117c92627e
📒 Files selected for processing (9)
pkg/api/job_runs.gopkg/db/query/cumulative_query.gopkg/db/query/cumulative_query_test.gopkg/db/views.gopkg/filter/filterable.gopkg/flags/postgres_benchmarking_test.gosippy-ng/src/jobs/JobRunsTable.jsxtest/integration/job_runs_report_test.gotest/integration/jobs_test.go
💤 Files with no reviewable changes (2)
- pkg/db/views.go
- pkg/flags/postgres_benchmarking_test.go
15fd099 to
f18213b
Compare
openshift-ci
Bot
added
the
needs-rebase
f18213b to
d189454
Compare
openshift-ci
Bot
removed
the
needs-rebase
Replace the materialized view with a two-phase direct query in JobsRunsReportFromDB. Phase 1 paginates from prow_job_runs JOIN prow_jobs with proper filtering and sorting. Phase 2 enriches the paginated IDs with test names, PR data, and annotations via errgroup. Key changes: - Use stored test_flakes column (from TRT-2834) instead of runtime subquery - Return ValidationError (400) for unsupported test name filter operators - Wrap OR-joined filter clauses in parentheses for correct SQL precedence - Add 20 integration tests covering pagination, sorting, filtering, enrichment, and error cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d189454 to
7cdc7cb
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
|
Scheduling required tests: |
|
@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. |
2 participants
Summary
prow_job_runs_report_matviewwith a two-phase direct query inJobsRunsReportFromDB: Phase 1 paginates from base tables (prow_job_runs+prow_jobs), Phase 2 enriches the paginated page with test name arrays, pull request data, and annotations viaerrgroup.PostgresMatViews, eliminating a ~90-second refresh that blocked data loading.test_failuresandtest_flakesdirectly from stored columns onprow_job_runs(populated at insert time by the prow loader), eliminating the runtime subqueries the matview used to compute these counts.pkg/filterto exportFilterItemToSQL(column-expression based) andFilterFieldToSQL(type-aware with array/timestamp handling), replacing the unexportedorFilterToSQL.sortable: falseto test name columns in the frontend and returnsValidationErrorfor unsortable sort fields.escapeLikeMetacharsinto an exportedfilter.EscapeLikeMetacharsand applies it at all 12 ILIKE pattern construction sites (FilterItemToSQL,FilterFieldToSQL,andFilterToSQL,testNameFilterSQL). Previously, user-provided%and_characters in filter values acted as ILIKE wildcards, causing unintended matches.Key design decisions
test_failuresandtest_flakesare read directly fromprow_job_runscolumns rather than computed via subqueries or CTEs. These columns are populated at insert time by the prow loader (TRT-2834), so no runtime aggregation is needed.COALESCE(col, '{}')to ensureNOTwrapping works correctly for NULL arrays.prow_job_run_testsrather than array column filters, matching the previous matview behavior.Test plan
make lint)make test)make integration) with 43 test functions covering:=,!=,>,>=,<,<=,contains,startsWith,endsWith,hasEntry,hasEntryContaining,isEmpty,isNotEmptyStaging verification (sippy-staging, release 4.19)
All 10 checks passed against the staging PostgreSQL database with real production-scale data:
job contains "aws")NOT job contains "aws")failed_test_names contains "etcd")e2e_awsvse2e-aws)release.openshift.io/architecture)job = "nonexistent-job-xyz")🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests