TRT-2768: Import /payload job results from PRs into postgres#3728
Open
dgoodwin wants to merge 6 commits into
Open
TRT-2768: Import /payload job results from PRs into postgres#3728dgoodwin wants to merge 6 commits into
dgoodwin wants to merge 6 commits into
Conversation
Contributor
|
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. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR normalizes payload Prow jobs, seeds matching presubmit data, and switches PR test results retrieval and serving to PostgreSQL-backed logic. ChangesPayload job and PR results flow
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[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}
Contributor
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
Contributor
|
Scheduling required tests: |
…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>
Contributor
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
Comment on lines
+81
to
+85
| 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) |
Contributor
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
Contributor
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
Comment on lines
+165
to
+193
| now := time.Now().UTC() | ||
| startDate := now.AddDate(0, 0, -14) | ||
| endDate := now.AddDate(0, 0, 1) | ||
|
|
||
| startDate, err := time.Parse("2006-01-02", startDateStr) | ||
| if err != nil { | ||
| RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{ | ||
| "code": http.StatusBadRequest, | ||
| "message": fmt.Sprintf("invalid start_date format (expected YYYY-MM-DD): %v", err), | ||
| }) | ||
| return | ||
| startDateStr := param.SafeRead(req, "start_date") | ||
| if startDateStr != "" { | ||
| parsed, err := time.Parse("2006-01-02", startDateStr) | ||
| if err != nil { | ||
| RespondWithJSON(http.StatusBadRequest, w, map[string]any{ | ||
| "code": http.StatusBadRequest, | ||
| "message": fmt.Sprintf("invalid start_date format (expected YYYY-MM-DD): %v", err), | ||
| }) | ||
| return | ||
| } | ||
| startDate = parsed | ||
| } | ||
|
|
||
| endDateStr := param.SafeRead(req, "end_date") | ||
| if endDateStr == "" { | ||
| RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{ | ||
| "code": http.StatusBadRequest, | ||
| "message": "required parameter 'end_date' is missing (format: YYYY-MM-DD)", | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| endDate, err := time.Parse("2006-01-02", endDateStr) | ||
| if err != nil { | ||
| RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{ | ||
| "code": http.StatusBadRequest, | ||
| "message": fmt.Sprintf("invalid end_date format (expected YYYY-MM-DD): %v", err), | ||
| }) | ||
| return | ||
| if endDateStr != "" { | ||
| parsed, err := time.Parse("2006-01-02", endDateStr) | ||
| if err != nil { | ||
| RespondWithJSON(http.StatusBadRequest, w, map[string]any{ | ||
| "code": http.StatusBadRequest, | ||
| "message": fmt.Sprintf("invalid end_date format (expected YYYY-MM-DD): %v", err), | ||
| }) | ||
| return | ||
| } | ||
| // Make end_date inclusive | ||
| endDate = parsed.AddDate(0, 0, 1) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Validate and cap the normalized date window before querying.
Explicit start_date/end_date values can now span arbitrarily large ranges, and start_date=2024-01-02&end_date=2024-01-01 normalizes to an empty half-open window that bypasses endDate.Before(startDate). Reintroduce the previous/documented cap and require the normalized end to be after the start before hitting prow_job_run_tests.
As per path instructions, “Validate at trust boundaries with allow-lists, not deny-lists.”
Proposed hardening
+const maxPRTestResultsDateRange = 31 * 24 * time.Hour
+
func PrintPRTestResultsJSON(w http.ResponseWriter, req *http.Request, dbc *db.DB) {
@@
- if endDate.Before(startDate) {
+ if !endDate.After(startDate) {
RespondWithJSON(http.StatusBadRequest, w, map[string]any{
"code": http.StatusBadRequest,
- "message": "end_date must be after start_date",
+ "message": "end_date must be on or after start_date",
})
return
}
+
+ if endDate.Sub(startDate) > maxPRTestResultsDateRange {
+ RespondWithJSON(http.StatusBadRequest, w, map[string]any{
+ "code": http.StatusBadRequest,
+ "message": "date range is too large",
+ })
+ return
+ }Also applies to: 196-202
🤖 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 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.
Source: Path instructions
Contributor
Author
|
/test lint |
Contributor
|
Scheduling required tests: |
Contributor
Author
|
/test e2e |
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>
Contributor
|
Scheduling required tests: |
Contributor
|
@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. |
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 by CodeRabbit
Summary by CodeRabbit
New Features
/api/pull_requests/test_results(defaults, filters, date ranges, and error/empty states).Bug Fixes
API Updates
/api/pull_requests/test_resultsnow always uses PostgreSQL, with updated response fields and filtering behavior (failures by default, optional successes andlatest_sha_only).