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. |
WalkthroughChangesJob runs reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JobsRunsReportFromDB
participant PostgreSQL
participant errgroup
JobsRunsReportFromDB->>PostgreSQL: Execute dynamic paginated job-run query
PostgreSQL-->>JobsRunsReportFromDB: Return base job-run rows
JobsRunsReportFromDB->>errgroup: Start concurrent enrichment tasks
errgroup->>PostgreSQL: Query flakes, test names, pull requests, and annotations
PostgreSQL-->>errgroup: Return enrichment data
errgroup-->>JobsRunsReportFromDB: Complete enriched report rows
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 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: 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
1157298 to
59c83ec
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
59c83ec to
d771bb9
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
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. |
|
@coderabbitai resolve |
openshift-ci
Bot
added
the
ready-for-human-review
✅ Action performedComments resolved and changes approved. |
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 counts, test name arrays, pull request data, and annotations viaerrgroup.PostgresMatViews, eliminating a ~90-second refresh that blocked data loading.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.Key design decisions
COALESCE(col, '{}')to ensureNOTwrapping works correctly for NULL arrays.prow_job_run_testsrather than array column filters, matching the previous matview behavior.Future work
flaked_test_countcolumn toprow_job_runs(populated at insert time by the prow loader), bringing flake count data on par with the existingtest_failurescolumn and eliminating the runtime subquery currently needed to compute flake counts.Test plan
make lint)make test)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Performance