TRT-2768: Import /payload job results from PRs into postgres#3728
TRT-2768: Import /payload job results from PRs into postgres#3728dgoodwin wants to merge 17 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
dgoodwin
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dgoodwin: This pull request references TRT-2768 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR normalizes payload Prow jobs as presubmits, adds synthetic presubmit data, and moves pull request test-result retrieval from BigQuery to PostgreSQL with updated endpoint handling and e2e coverage. ChangesPayload presubmit classification and normalization
PostgreSQL PR test results endpoint
Synthetic presubmit data and endpoint validation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 18 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (18 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/dataloader/prowloader/prow.go (1)
1002-1035: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep payload
TestGridURLsuppressed on updates too.New payload jobs skip
TestGridURL, but the existing-job path later backfills it whenever the field is empty. That makes the payload-specific behavior disappear after a subsequent import.Proposed fix
- if len(dbProwJob.TestGridURL) == 0 { + if isPayloadPresubmit && dbProwJob.TestGridURL != "" { + dbProwJob.TestGridURL = "" + saveDB = true + } else if !isPayloadPresubmit && len(dbProwJob.TestGridURL) == 0 { dbProwJob.TestGridURL = pl.generateTestGridURL(release, pj.Spec.Job).String() if len(dbProwJob.TestGridURL) > 0 { saveDB = true }🤖 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/dataloader/prowloader/prow.go` around lines 1002 - 1035, The existing-job update path in the prow loader is backfilling TestGridURL even for payload jobs, which breaks the payload-specific suppression. Update the logic in the prow job import flow around pl.generateTestGridURL and dbProwJob handling so TestGridURL is only populated for non-payload jobs, both on insert and on subsequent updates, and leave empty values untouched for payload imports.
🧹 Nitpick comments (2)
pkg/dataloader/prowloader/prow.go (1)
975-1020: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd regression tests for payload variant and persistence behavior.
Please cover
releaseJobName-based variant identification, release variant rewriting toPresubmits, create-vs-update behavior, and TestGridURL suppression. As per coding guidelines, modified Go functionality should include test coverage.🤖 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/dataloader/prowloader/prow.go` around lines 975 - 1020, Add regression tests around prow job persistence in the prow loader to cover the new payload handling in identifyVariants and the dbProwJob create/update path. Verify that when pj.Annotations["releaseJobName"] is present and pj.Spec.Refs is non-nil, the variant name comes from releaseJobName, any release variant is rewritten to Presubmits, and TestGridURL is left empty; also cover the non-payload case where generateTestGridURL is used. Include both the new ProwJob creation path and the existing cache/update path in the prowJobCache logic so the behavior stays covered.Source: Coding guidelines
pkg/dataloader/prowloader/bigqueryjobs.go (1)
132-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd regression coverage for the payload transformation contract.
This new branch rewrites identity, type, annotations, and PR refs. Please add tests for stable-name stripping, fallback naming, aggregator skipping, and comma-normalized
releaseJobNamevalues. As per coding guidelines, new or modified functionality should include test coverage.🤖 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/dataloader/prowloader/bigqueryjobs.go` around lines 132 - 176, Add regression tests for the payload sub-job transformation in the bigquery job ingestion path, covering the branch that rewrites `jobName` and `jobType` when `releaseJobName` is present. Exercise stable-name stripping from `bqjr.JobName`, the `payload-pr-` fallback when the PR prefix cannot be removed, skipping jobs whose names start with `aggregator-`, and normalization of comma-separated `releaseJobName` values. Add assertions around the resulting `prow.ProwJob` fields produced by the `prowJobs` population logic so the identity and type contract stays covered.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/dataloader/prowloader/bigqueryjobs.go`:
- Around line 139-156: The fallback path in bigqueryjobs.go uses the raw
releaseJobName annotation, which can include comma-delimited suffixes and lead
to incorrect canonical job names. Normalize releaseJobName the same way the
BigQuery query-generator does by trimming everything after the first comma
before building stableName or any downstream variant identifiers. Apply this in
the releaseJobName handling block near the stableName fallback so all consumers
see the normalized value.
---
Outside diff comments:
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 1002-1035: The existing-job update path in the prow loader is
backfilling TestGridURL even for payload jobs, which breaks the payload-specific
suppression. Update the logic in the prow job import flow around
pl.generateTestGridURL and dbProwJob handling so TestGridURL is only populated
for non-payload jobs, both on insert and on subsequent updates, and leave empty
values untouched for payload imports.
---
Nitpick comments:
In `@pkg/dataloader/prowloader/bigqueryjobs.go`:
- Around line 132-176: Add regression tests for the payload sub-job
transformation in the bigquery job ingestion path, covering the branch that
rewrites `jobName` and `jobType` when `releaseJobName` is present. Exercise
stable-name stripping from `bqjr.JobName`, the `payload-pr-` fallback when the
PR prefix cannot be removed, skipping jobs whose names start with `aggregator-`,
and normalization of comma-separated `releaseJobName` values. Add assertions
around the resulting `prow.ProwJob` fields produced by the `prowJobs` population
logic so the identity and type contract stays covered.
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 975-1020: Add regression tests around prow job persistence in the
prow loader to cover the new payload handling in identifyVariants and the
dbProwJob create/update path. Verify that when pj.Annotations["releaseJobName"]
is present and pj.Spec.Refs is non-nil, the variant name comes from
releaseJobName, any release variant is rewritten to Presubmits, and TestGridURL
is left empty; also cover the non-payload case where generateTestGridURL is
used. Include both the new ProwJob creation path and the existing cache/update
path in the prowJobCache logic so the behavior stays covered.
🪄 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: c6a2fb07-147f-4982-809e-c476b82a5c71
📒 Files selected for processing (2)
pkg/dataloader/prowloader/bigqueryjobs.gopkg/dataloader/prowloader/prow.go
|
Scheduling required tests: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/prtestresults.go (1)
42-124: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd test coverage for GetPRTestResults and PrintPRTestResultsJSON functions.
Per coding guidelines, new functionality requires unit tests. The PostgreSQL query in GetPRTestResults (lines 42–124) contains complex JOIN logic, conditional filtering, and date bounds that should be covered by tests. The HTTP handler PrintPRTestResultsJSON (lines 126–220) should also be tested. Add tests covering: default failure-only results, include_successes pattern matching, output joins, date boundary filtering, and invalid input handling.
🤖 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/prtestresults.go` around lines 42 - 124, Add unit tests for GetPRTestResults and PrintPRTestResultsJSON to cover the new query and handler behavior. Focus on GetPRTestResults’s JOIN/filter logic by testing default failure-only results, includeSuccesses pattern matching, output join population, and start/end date boundary handling using the function name and its query-building flow to locate the code. Also add handler tests for PrintPRTestResultsJSON to verify valid responses and invalid input handling, ensuring the JSON endpoint exercises the same PR test result path.Source: Coding guidelines
🧹 Nitpick comments (1)
cmd/sippy/seed_data.go (1)
903-963: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse status constants and align the comments with the seeded tests.
The seed rows use raw status integers and two comments describe different tests than the
TestIDactually assigned. This makes the API fixture easy to misread.Proposed cleanup
- // Failure result for install test + // Seed an install failure so the default PR test-results view has data. failResult := models.ProwJobRunTest{ @@ - Status: 12, + Status: int(v1.TestStatusFailure), @@ - // Success result for install test (same test, different run aspect) + // Seed a network success so include_successes exercises non-failure statuses. successResult := models.ProwJobRunTest{ @@ - Status: 1, + Status: int(v1.TestStatusSuccess), @@ - // Flake result for network test on a different test + // Seed an install flake so include_successes exercises flakes. flakeResult := models.ProwJobRunTest{ @@ - Status: 13, + Status: int(v1.TestStatusFlake),As per coding guidelines, “Follow idiomatic Go practices” and keep comments minimal/helpful.
🤖 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/seed_data.go` around lines 903 - 963, The seeded ProwJobRunTest rows are hard to read because they use raw status integers and the comments don’t match the actual TestID being set. Update the seeding logic to use the existing status constants instead of literal values, and revise or remove the comments so they accurately describe each result created via failResult, successResult, and flakeResult with installTestID/networkTestID. Keep the fixture idiomatic and minimal so the test data is self-explanatory.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/prtestresults.go`:
- Around line 81-85: Normalize and validate include_successes before
constructing the query in the prtestresults filtering logic, since the current
loop in the query builder can produce unrestricted LIKE matches. In the code
around the conditions assembly, trim out empty entries, enforce a reasonable
maximum count and per-item length, and escape SQL LIKE wildcard characters
before passing patterns into the Or("t.name LIKE ?", ...) clauses. Apply the
same hardening to the related include_successes handling referenced by the
second occurrence so the endpoint only accepts safe, bounded values.
- Around line 210-215: The error handling in the PR test results path is leaking
backend error details to clients. Update the error branch in the PR test results
handler to keep the detailed failure only in the `log.WithError(err).Error(...)`
call and return a generic HTTP 500 JSON body from `RespondWithJSON` without
embedding `err` in the `message` field.
---
Outside diff comments:
In `@pkg/api/prtestresults.go`:
- Around line 42-124: Add unit tests for GetPRTestResults and
PrintPRTestResultsJSON to cover the new query and handler behavior. Focus on
GetPRTestResults’s JOIN/filter logic by testing default failure-only results,
includeSuccesses pattern matching, output join population, and start/end date
boundary handling using the function name and its query-building flow to locate
the code. Also add handler tests for PrintPRTestResultsJSON to verify valid
responses and invalid input handling, ensuring the JSON endpoint exercises the
same PR test result path.
---
Nitpick comments:
In `@cmd/sippy/seed_data.go`:
- Around line 903-963: The seeded ProwJobRunTest rows are hard to read because
they use raw status integers and the comments don’t match the actual TestID
being set. Update the seeding logic to use the existing status constants instead
of literal values, and revise or remove the comments so they accurately describe
each result created via failResult, successResult, and flakeResult with
installTestID/networkTestID. Keep the fixture idiomatic and minimal so the test
data is self-explanatory.
🪄 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: ad4774e2-3201-48f9-a252-0e2a78f221d5
📒 Files selected for processing (5)
cmd/sippy/seed_data.gopkg/api/prtestresults.gopkg/dataloader/prowloader/bigqueryjobs.gopkg/dataloader/prowloader/prow.gopkg/sippyserver/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/dataloader/prowloader/bigqueryjobs.go
- pkg/dataloader/prowloader/prow.go
| conditions := dbc.DB.Where("pjrt.status = ?", int(sippyprocessingv1.TestStatusFailure)) | ||
| for _, pattern := range includeSuccesses { | ||
| conditions = conditions.Or("t.name LIKE ?", "%"+pattern+"%") | ||
| } | ||
| whereClause += ` | ||
| ) | ||
| )` | ||
| } | ||
| whereClause += ` | ||
| )` | ||
|
|
||
| queryString := fmt.Sprintf(` | ||
| WITH deduped_testcases AS ( | ||
| SELECT | ||
| junit.*, | ||
| ROW_NUMBER() OVER(PARTITION BY prowjob_build_id, file_path, test_name, testsuite ORDER BY | ||
| CASE | ||
| WHEN flake_count > 0 THEN 0 | ||
| WHEN success_val > 0 THEN 1 | ||
| ELSE 2 | ||
| END) AS row_num, | ||
| CASE | ||
| WHEN flake_count > 0 THEN 0 | ||
| ELSE success_val | ||
| END AS adjusted_success_val, | ||
| CASE | ||
| WHEN flake_count > 0 THEN 1 | ||
| ELSE 0 | ||
| END AS adjusted_flake_count | ||
| FROM | ||
| %s.%s AS junit | ||
| WHERE | ||
| junit.modified_time >= DATETIME(@StartDate) | ||
| AND junit.modified_time < DATETIME(@EndDate) | ||
| AND junit.skipped = false | ||
| ) | ||
| SELECT | ||
| jobs.prowjob_build_id, | ||
| jobs.prowjob_job_name AS prowjob_name, | ||
| jobs.prowjob_url, | ||
| jobs.pr_sha, | ||
| jobs.prowjob_start, | ||
| deduped.test_name, | ||
| deduped.testsuite, | ||
| CASE | ||
| WHEN deduped.adjusted_flake_count > 0 THEN TRUE | ||
| ELSE FALSE | ||
| END AS flaked, | ||
| CASE | ||
| WHEN deduped.adjusted_flake_count > 0 THEN TRUE | ||
| WHEN deduped.adjusted_success_val > 0 THEN TRUE | ||
| ELSE FALSE | ||
| END AS success, | ||
| deduped.failure_content | ||
| FROM | ||
| %s.jobs AS jobs | ||
| INNER JOIN | ||
| deduped_testcases AS deduped | ||
| ON | ||
| jobs.prowjob_build_id = deduped.prowjob_build_id | ||
| AND deduped.row_num = 1 | ||
| WHERE | ||
| jobs.org = @Org | ||
| AND jobs.repo = @Repo | ||
| AND jobs.pr_number = @PRNumber | ||
| AND jobs.prowjob_start >= DATETIME(@StartDate) | ||
| AND jobs.prowjob_start < DATETIME(@EndDate)%s | ||
| ORDER BY | ||
| jobs.prowjob_start DESC, | ||
| deduped.test_name ASC | ||
| `, bqc.Dataset, junitTable, bqc.Dataset, whereClause) | ||
|
|
||
| query := bqc.BQ.Query(queryString) | ||
| query.Parameters = []bigquery.QueryParameter{ | ||
| { | ||
| Name: "Org", | ||
| Value: org, | ||
| }, | ||
| { | ||
| Name: "Repo", | ||
| Value: repo, | ||
| }, | ||
| { | ||
| Name: "PRNumber", | ||
| Value: strconv.Itoa(prNumber), | ||
| }, | ||
| { | ||
| Name: "StartDate", | ||
| Value: startDate, | ||
| }, | ||
| { | ||
| Name: "EndDate", | ||
| Value: endDate, | ||
| }, | ||
| } | ||
|
|
||
| // Add parameters for includeSuccesses LIKE clauses | ||
| for i, testName := range includeSuccesses { | ||
| query.Parameters = append(query.Parameters, bigquery.QueryParameter{ | ||
| Name: fmt.Sprintf("IncludeSuccess%d", i), | ||
| Value: "%" + testName + "%", // Wrap in % for SQL LIKE partial matching | ||
| query = query.Where(conditions) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Normalize include_successes before building LIKE clauses.
?include_successes= produces LIKE '%%', and % / _ values can intentionally match every test name. Since the endpoint no longer uses param.SafeRead for this list parameter, trim empty values, cap count/length, and escape LIKE wildcards before querying.
Proposed hardening
+func escapeLikePattern(pattern string) string {
+ replacer := strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`)
+ return replacer.Replace(pattern)
+}
+
@@
} else {
conditions := dbc.DB.Where("pjrt.status = ?", int(sippyprocessingv1.TestStatusFailure))
for _, pattern := range includeSuccesses {
- conditions = conditions.Or("t.name LIKE ?", "%"+pattern+"%")
+ conditions = conditions.Or("t.name LIKE ? ESCAPE '\\'", "%"+escapeLikePattern(pattern)+"%")
}
query = query.Where(conditions)
}
@@
- includeSuccesses := req.URL.Query()["include_successes"]
+ includeSuccesses := make([]string, 0, len(req.URL.Query()["include_successes"]))
+ for _, pattern := range req.URL.Query()["include_successes"] {
+ pattern = strings.TrimSpace(pattern)
+ if pattern != "" {
+ includeSuccesses = append(includeSuccesses, pattern)
+ }
+ }As per path instructions, “Validate at trust boundaries with allow-lists, not deny-lists.”
Also applies to: 207-209
🤖 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/prtestresults.go` around lines 81 - 85, Normalize and validate
include_successes before constructing the query in the prtestresults filtering
logic, since the current loop in the query builder can produce unrestricted LIKE
matches. In the code around the conditions assembly, trim out empty entries,
enforce a reasonable maximum count and per-item length, and escape SQL LIKE
wildcard characters before passing patterns into the Or("t.name LIKE ?", ...)
clauses. Apply the same hardening to the related include_successes handling
referenced by the second occurrence so the endpoint only accepts safe, bounded
values.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/api/prtestresults.go (1)
208-213: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winKeep backend error details out of 500 responses.
Line 212 still echoes the wrapped DB error to clients. Keep the detailed error in
log.WithError(err)and return a generic message body.Proposed fix
RespondWithJSON(http.StatusInternalServerError, w, map[string]any{ "code": http.StatusInternalServerError, - "message": fmt.Sprintf("error fetching test results: %v", err), + "message": "error fetching test results", })🤖 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/prtestresults.go` around lines 208 - 213, The PR test results error path in the handler should not leak backend details in the HTTP 500 response body. In the same block that logs with log.WithError(err) inside the PR test results handler, change the RespondWithJSON payload to return a generic message only, while keeping the full wrapped error detail in the log for debugging.
🤖 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/prtestresults.go`:
- Around line 165-193: The date range handling in prtestresults should validate
the normalized window before querying, since explicit start_date and end_date
can now create arbitrarily large ranges or an empty half-open interval after
end_date is made inclusive. In the request-handling logic around param.SafeRead,
the time.Parse blocks, and the prow_job_run_tests query setup, reintroduce the
previous/documented maximum span cap and reject requests where the normalized
endDate is not after startDate before continuing. Keep the existing inclusive
end-date behavior, but ensure the final normalized bounds are checked as a valid
allow-listed window before any query is executed.
---
Duplicate comments:
In `@pkg/api/prtestresults.go`:
- Around line 208-213: The PR test results error path in the handler should not
leak backend details in the HTTP 500 response body. In the same block that logs
with log.WithError(err) inside the PR test results handler, change the
RespondWithJSON payload to return a generic message only, while keeping the full
wrapped error detail in the log for debugging.
🪄 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: 88ca46e1-07a1-41b0-b785-829de19913ab
📒 Files selected for processing (2)
pkg/api/prtestresults.gotest/e2e/pull_requests_test.go
|
/test lint |
|
Scheduling required tests: |
|
/test e2e |
|
Scheduling required tests: |
mstaeble
left a comment
mstaeble
left a comment
There was a problem hiding this comment.
We have an in-flight PR that's relevant here: #3679 (TRT-2734) adds a release_definitions table in PostgreSQL with a Capabilities field and constants like CapPullRequests. It provides the infrastructure to formally define what each release supports.
This PR introduces "Presubmits" as a pseudo-release with the name hardcoded throughout (prow loader routing, PG query filters, seed data, variant overrides). If both PRs land independently, "Presubmits" would exist in prow_jobs.release but not in release_definitions, which could cause issues as the server starts relying on release_definitions to enumerate releases or gate features.
A few things worth considering:
- "Presubmits" should probably get a ReleaseDefinition row (with CapPullRequests at minimum) so it's a first-class release rather than a magic string that only exists implicitly.
- The CapPullRequests constant in #3679 is currently unused. This endpoint would be a natural consumer for capability-based gating.
- The seed data here should eventually seed a "Presubmits" ReleaseDefinition alongside the presubmit jobs/runs.
Not necessarily something to solve in this PR, but wanted to flag the overlap so we can coordinate on how "Presubmits" gets defined as a release. Happy to discuss sequencing.
| } | ||
| } | ||
|
|
||
| // Success result for install test (same test, different run aspect) |
There was a problem hiding this comment.
Is this comment accurate? Is says it is for the same install test, but the TestID used is "networkTestID".
| ProwJobRunTimestamp: ri.run.Timestamp, | ||
| TestID: installTestID, | ||
| SuiteID: &suite.ID, | ||
| Status: 12, |
There was a problem hiding this comment.
Should we use the constants for Status?
| COALESCE(s.name, '') AS test_suite, | ||
| pjrt.status, | ||
| COALESCE(pjrto.output, '') AS output`). | ||
| Joins("JOIN prow_job_run_prow_pull_requests jrpr ON jrpr.prow_pull_request_id = pp.id"). |
There was a problem hiding this comment.
Two performance concerns with this join:
Missing index: The primary key on prow_job_run_prow_pull_requests is (prow_job_run_id, prow_pull_request_id). Since prow_pull_request_id is the second column, PostgreSQL can't use the PK index for this jrpr.prow_pull_request_id = pp.id lookup and must sequentially scan the join table. This table grows by one row per job-run-to-PR association. A single-column index on prow_pull_request_id would fix this.
Missing release/timestamp filter on join table: Every other query in the codebase that uses this table filters on its denormalized prow_job_run_release and prow_job_run_timestamp columns (see pull_request_queries.go:32, repository_queries.go:30, functions.go:89-90). There's a composite index idx_prow_job_run_prow_pull_requests_release_timestamp specifically for this. Adding these conditions to the join would let the planner use it:
Joins("JOIN prow_job_run_prow_pull_requests jrpr ON
jrpr.prow_pull_request_id = pp.id AND
jrpr.prow_job_run_release = 'Presubmits' AND
jrpr.prow_job_run_timestamp >= ? AND
jrpr.prow_job_run_timestamp < ?",
startDate, endDate)
| // Start from the PR side and use partition keys (release, timestamp) on | ||
| // prow_job_run_tests to allow PostgreSQL to prune partitions and avoid | ||
| // locking every partition in the table. | ||
| query := dbc.DB.Table("prow_pull_requests pp"). |
There was a problem hiding this comment.
Using dbc.DB.Table(...) with a raw table name bypasses GORM's automatic WHERE deleted_at IS NULL injection. ProwPullRequest, ProwJobRun, and ProwJob all have soft-delete support, so soft-deleted records could leak into results. Other queries in the codebase (e.g., recent_test_failures.go:38-41) add explicit deleted_at IS NULL conditions on each table when using raw table names. Consider adding:
Where("pp.deleted_at IS NULL AND pjr.deleted_at IS NULL AND pj.deleted_at IS NULL")
| Joins("JOIN tests t ON t.id = pjrt.test_id"). | ||
| Joins("LEFT JOIN suites s ON s.id = pjrt.suite_id"). | ||
| Joins("LEFT JOIN prow_job_run_test_outputs pjrto ON pjrto.prow_job_run_test_id = pjrt.id AND pjrto.prow_job_run_test_timestamp = pjrt.prow_job_run_timestamp AND pjrto.prow_job_run_test_release = pjrt.prow_job_run_release"). | ||
| Where("pp.org = ? AND pp.repo = ? AND pp.number = ?", org, repo, prNumber). |
There was a problem hiding this comment.
There's no index on prow_pull_requests(org, repo, number). The model only has a unique index on (sha, link). This filter is the primary selectivity driver for the entire query (a specific PR typically has 1-5 rows), but without an index PostgreSQL must sequentially scan the table. As the table grows with each unique (PR, SHA) combination, this will get progressively slower.
Consider adding a composite index to the model:
Org string `json:"org" gorm:"index:idx_prow_pull_requests_org_repo_number"`
Repo string `json:"repo" gorm:"index:idx_prow_pull_requests_org_repo_number"`
Number int `json:"number" gorm:"index:idx_prow_pull_requests_org_repo_number"`
| query = query.Where(conditions) | ||
| } | ||
|
|
||
| query = query.Order("pjr.timestamp DESC, t.name ASC") |
There was a problem hiding this comment.
There's no result limit or pagination. The default date range is 14 days, and a PR tested many times with many tests could return thousands of rows. The old BigQuery version had a 30-day cap as a safeguard. Consider adding a configurable limit, or at minimum a reasonable ceiling to protect against unexpectedly large result sets.
| } else { | ||
| conditions := dbc.DB.Where("pjrt.status = ?", int(sippyprocessingv1.TestStatusFailure)) | ||
| for _, pattern := range includeSuccesses { | ||
| conditions = conditions.Or("t.name LIKE ?", "%"+pattern+"%") |
There was a problem hiding this comment.
Minor: the OR t.name LIKE '%pattern%' matches regardless of status, so it returns successes, flakes, and failures for matching test names. The old BigQuery version only included successes for matching names (explicitly excluded flakes via adjusted_flake_count = 0). The e2e test confirms this is intentional (statuses["success"]+statuses["flake"] > 0), but it's a behavior change worth noting in the PR description for anyone consuming this API.
…kfill Normalize releaseJobName by stripping comma-delimited suffixes to match the BigQuery query generator behavior. Also prevent the TestGridURL update path from backfilling a URL on payload presubmit jobs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
start_date and end_date are now optional (default to last 14 days). Added sha query param to filter results to a specific commit. Updated e2e tests accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The sha param caused a server crash because it was not registered in the param.SafeRead registry (which calls log.Fatal for unknown params). Instead, use a latest_sha_only boolean param that filters results to the SHA from the most recent prow job run, removing the burden from the caller to know the exact SHA. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # pkg/api/job_runs.go # pkg/dataloader/prowloader/prow.go
The comment said "install test" but the code uses networkTestID. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace raw int status values (1, 12, 13) with the named constants from sippyprocessingv1 (TestStatusSuccess, TestStatusFailure, TestStatusFlake). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review feedback: - Add release/timestamp filters to the prow_job_run_prow_pull_requests join so PostgreSQL can use the composite index idx_prow_job_run_prow_pull_requests_release_timestamp. - Add explicit deleted_at IS NULL conditions for soft-delete safety since raw table names bypass GORM's automatic injection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The PR test results query filters primarily on org/repo/number but had no supporting index, requiring a sequential scan as the table grows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Default to 10000 rows max, configurable via a limit query parameter. Prevents unexpectedly large result sets from PRs with many test runs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous implementation returned all statuses (including flakes) for test names matching include_successes patterns. The old BigQuery version only included successes. Restrict the OR clause to status=success for matching test names. Also add an install success result to the presubmit seed data so the e2e test can verify the behavior, and update the e2e assertion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d5763a0 to
5662a6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/variantregistry/ocp.go (1)
763-768: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCover both
-iso-no-registryclassifications.Add table cases asserting its capability,
candidatetier, andmetalplatform; these separate ordered match tables can otherwise drift unnoticed.As per coding guidelines, “New or modified functionality should include test coverage.”
Also applies to: 1139-1141
🤖 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/variantregistry/ocp.go` around lines 763 - 768, Extend the tests for the component capability pattern tables to cover both `-iso-no-registry` classifications, asserting the expected capability, `candidate` tier, and `metal` platform for each ordered match table. Locate the relevant table-driven tests near the existing `-iso-no-registry` entry and add cases for both classifications.Source: Coding guidelines
pkg/api/job_runs.go (1)
374-383: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude
Presubmitsfrom the comparison baseline.
GetReleasesFromDBordersdevelopment_start_date DESC, where PostgreSQL places nulls first. Since the Presubmits definition has no development start date,ar[0]can bePresubmits, leaving the comparison release unchanged. Select the first non-presubmit release or query withNULLS LASTwhile excluding the pseudo-release.🤖 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 374 - 383, Update the compareRelease handling in the ReleasePresubmits branch to exclude the Presubmits pseudo-release when selecting the baseline from GetReleasesFromDB. Choose the first non-presubmit release, preserve the existing no-releases error when none exists, and assign that release to compareRelease.
🤖 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 `@cmd/sippy/seed_data.go`:
- Around line 969-1035: Update the seed data around the ProwJobRun creation and
join-table linking so PR 99001’s runs use competing SHAs: assign older runs an
older SHA and the newest run the current SHA, while preserving PR 99002’s
existing behavior. Ensure the seeded relationships support the latest_sha_only
e2e assertion that only the newest SHA is returned.
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 687-701: Update the payload variant rewrite in the isPayload
branch after IdentifyVariants: parse each variant’s key:value form, compare the
value portion with pl.config.Releases, and preserve the key when replacing the
value. Rewrite matching entries as Release:Presubmits rather than a bare
Presubmits string.
- Around line 604-645: Add unit tests for matchRelease and isPayloadPresubmit
covering all annotation/refs combinations, including annotated jobs with and
without refs and unannotated jobs with and without refs. Verify payload
presubmits resolve to models.ReleasePresubmits only when that release is
enabled, and return no match when Presubmits is disabled.
In `@pkg/util/param/param.go`:
- Around line 79-80: Update the pull request test results limit handling in
SafeRead to use ReadUint with defaultPRTestResultsLimit, and propagate its
validation error so malformed, zero, or over-limit values return 400 instead of
being converted to empty or reset to 10,000.
---
Outside diff comments:
In `@pkg/api/job_runs.go`:
- Around line 374-383: Update the compareRelease handling in the
ReleasePresubmits branch to exclude the Presubmits pseudo-release when selecting
the baseline from GetReleasesFromDB. Choose the first non-presubmit release,
preserve the existing no-releases error when none exists, and assign that
release to compareRelease.
In `@pkg/variantregistry/ocp.go`:
- Around line 763-768: Extend the tests for the component capability pattern
tables to cover both `-iso-no-registry` classifications, asserting the expected
capability, `candidate` tier, and `metal` platform for each ordered match table.
Locate the relevant table-driven tests near the existing `-iso-no-registry`
entry and add cases for both classifications.
🪄 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: e7de0aa6-5cfd-4160-bb34-595ff76243cc
📒 Files selected for processing (11)
cmd/sippy/seed_data.gopkg/api/job_runs.gopkg/api/prtestresults.gopkg/dataloader/prowloader/bigqueryjobs.gopkg/dataloader/prowloader/prow.gopkg/db/models/prow.gopkg/db/models/releases.gopkg/sippyserver/server.gopkg/util/param/param.gopkg/variantregistry/ocp.gotest/e2e/pull_requests_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/dataloader/prowloader/bigqueryjobs.go
- pkg/sippyserver/server.go
- test/e2e/pull_requests_test.go
- pkg/api/prtestresults.go
| // Create ProwPullRequests | ||
| prs := []models.ProwPullRequest{ | ||
| { | ||
| Org: "openshift", | ||
| Repo: "origin", | ||
| Number: 99001, | ||
| Author: "test-author-1", | ||
| Title: "Test PR 99001", | ||
| SHA: "abc123def456", | ||
| Link: "https://github.com/openshift/origin/pull/99001", | ||
| }, | ||
| { | ||
| Org: "openshift", | ||
| Repo: "origin", | ||
| Number: 99002, | ||
| Author: "test-author-2", | ||
| Title: "Test PR 99002", | ||
| SHA: "789abc012def", | ||
| Link: "https://github.com/openshift/origin/pull/99002", | ||
| }, | ||
| } | ||
|
|
||
| for i, pr := range prs { | ||
| if err := dbc.DB.Create(&pr).Error; err != nil { | ||
| return fmt.Errorf("failed to create ProwPullRequest %d: %w", pr.Number, err) | ||
| } | ||
| prs[i] = pr | ||
| } | ||
|
|
||
| // Create runs: 3 runs per job, PR 99001 gets job[0] runs, PR 99002 gets job[1] runs | ||
| type runInfo struct { | ||
| run models.ProwJobRun | ||
| prIdx int | ||
| } | ||
| var runs []runInfo | ||
|
|
||
| for jobIdx, pj := range presubmitJobs { | ||
| for i := 0; i < 3; i++ { | ||
| timestamp := now.Add(-time.Duration(3-i) * 20 * time.Hour) | ||
| run := models.ProwJobRun{ | ||
| ProwJobID: pj.ID, | ||
| ProwJobRelease: models.ReleasePresubmits, | ||
| Cluster: "build01", | ||
| Timestamp: timestamp, | ||
| Duration: 2 * time.Hour, | ||
| OverallResult: v1.JobTestFailure, | ||
| Failed: true, | ||
| } | ||
| if err := dbc.DB.Create(&run).Error; err != nil { | ||
| return fmt.Errorf("failed to create ProwJobRun: %w", err) | ||
| } | ||
| runs = append(runs, runInfo{run: run, prIdx: jobIdx}) | ||
| } | ||
| } | ||
|
|
||
| // Link runs to PRs via join table | ||
| for _, ri := range runs { | ||
| jrpr := models.ProwJobRunProwPullRequest{ | ||
| ProwJobRunID: ri.run.ID, | ||
| ProwPullRequestID: prs[ri.prIdx].ID, | ||
| ProwJobRunRelease: models.ReleasePresubmits, | ||
| ProwJobRunTimestamp: ri.run.Timestamp, | ||
| } | ||
| if err := dbc.DB.Create(&jrpr).Error; err != nil { | ||
| return fmt.Errorf("failed to create ProwJobRunProwPullRequest: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Seed competing SHAs for PR 99001.
Every run currently links to the same SHA, so the latest_sha_only e2e test passes even if that filter is a no-op. Link older runs to an older SHA and assert that only the newest SHA is returned.
🤖 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/seed_data.go` around lines 969 - 1035, Update the seed data around
the ProwJobRun creation and join-table linking so PR 99001’s runs use competing
SHAs: assign older runs an older SHA and the newest run the current SHA, while
preserving PR 99002’s existing behavior. Ensure the seeded relationships support
the latest_sha_only e2e assertion that only the newest SHA is returned.
| @@ -628,6 +638,12 @@ func (pl *ProwLoader) matchRelease(jobName string) string { | |||
| return "" | |||
| } | |||
|
|
|||
| // isPayloadPresubmit returns true if the prow job is a /payload sub-job. | |||
| func isPayloadPresubmit(pj *prow.ProwJob) bool { | |||
| _, hasAnnotation := pj.Annotations["releaseJobName"] | |||
| return hasAnnotation && pj.Spec.Refs != nil | |||
| } | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add unit coverage for payload-presubmit detection.
The endpoint e2e seed writes presubmit rows directly, so it does not exercise matchRelease or isPayloadPresubmit. Cover annotation/refs combinations and behavior when Presubmits is disabled.
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/dataloader/prowloader/prow.go` around lines 604 - 645, Add unit tests for
matchRelease and isPayloadPresubmit covering all annotation/refs combinations,
including annotated jobs with and without refs and unannotated jobs with and
without refs. Verify payload presubmits resolve to models.ReleasePresubmits only
when that release is enabled, and return no match when Presubmits is disabled.
Source: Coding guidelines
| variantJobName := pj.Spec.Job | ||
| isPayload := isPayloadPresubmit(pj) | ||
| if isPayload { | ||
| variantJobName = pj.Annotations["releaseJobName"] | ||
| } | ||
|
|
||
| variants := pl.variantManager.IdentifyVariants(variantJobName) | ||
| if isPayload { | ||
| for vi, v := range variants { | ||
| if _, isRel := pl.config.Releases[v]; isRel { | ||
| variants[vi] = models.ReleasePresubmits | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the serialized Release: variant key.
ProwJob.Variants entries use key:value form, but Line 696 compares the complete entry such as Release:4.22 against the config key 4.22; Line 697 would also replace it with bare Presubmits. Consequently, payload jobs retain the versioned release variant or receive a malformed one. Parse the entry and rewrite it as Release:Presubmits.
🤖 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/dataloader/prowloader/prow.go` around lines 687 - 701, Update the payload
variant rewrite in the isPayload branch after IdentifyVariants: parse each
variant’s key:value form, compare the value portion with pl.config.Releases, and
preserve the key when replacing the value. Rewrite matching entries as
Release:Presubmits rather than a bare Presubmits string.
| // pull request test results params | ||
| "limit": uintRegexp, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return 400 for invalid or excessive limits.
SafeRead silently converts malformed input to empty, while 0 or values above 10,000 are later reset to 10,000. Use ReadUint(req, "limit", defaultPRTestResultsLimit) and propagate its validation error instead of unexpectedly requesting the maximum result set.
🤖 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/util/param/param.go` around lines 79 - 80, Update the pull request test
results limit handling in SafeRead to use ReadUint with
defaultPRTestResultsLimit, and propagate its validation error so malformed,
zero, or over-limit values return 400 instead of being converted to empty or
reset to 10,000.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The PK is (prow_job_run_id, prow_pull_request_id) so PostgreSQL cannot use it for lookups starting from the pull request side. Add a single-column index so the PR test results query can efficiently join from prow_pull_requests into the association table. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling required tests: |
|
@dgoodwin: 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 by CodeRabbit
Summary by CodeRabbit
New Features
/api/pull_requests/test_results.limitquery parameter.Bug Fixes
API Updates
/api/pull_requests/test_resultsnow always uses PostgreSQL and returns job-run–scoped results, with failures-only-by-default plus optional successes (include_successes) andlatest_sha_only.