Skip to content

fix(ci-failures): avoid removing non matching expressions#108

Merged
dhiller merged 2 commits intokubevirt:mainfrom
dhiller:fix-filtering-ci-report
Mar 10, 2026
Merged

fix(ci-failures): avoid removing non matching expressions#108
dhiller merged 2 commits intokubevirt:mainfrom
dhiller:fix-filtering-ci-report

Conversation

@dhiller
Copy link
Contributor

@dhiller dhiller commented Mar 6, 2026

What this PR does / why we need it:

The regular expression silently filtered some urls that weren't matching (i.e. pull-kubevirt-e2e-kind-sriov) was not matching.

Since we basically don't need the sorting it's removed, now we are directly checking whether the junit is present.

Also any HEAD error was treated as a non-existing junit xml which might cover some errors - these are now returned.

Finally min and max are removed in favor of the built-ins.

/kind bug

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

/cc @dollierp

@kubevirt-bot kubevirt-bot requested a review from dollierp March 6, 2026 14:35
@kubevirt-bot kubevirt-bot added kind/bug dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Mar 6, 2026
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • ShowCIFailureJobs now aborts on the first HEAD error, whereas previously those were treated like a missing junit and iteration continued; if transient GCS or network issues are common, consider logging the error and skipping that URL (or aggregating errors) instead of failing the entire call.
  • checkJunitFuncTestXMLExists hardcodes the prow-to-GCS URL replacement logic; consider extracting the prefix constants or sharing existing helpers so future changes to prow or storage paths don’t require updating this function in isolation.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- ShowCIFailureJobs now aborts on the first HEAD error, whereas previously those were treated like a missing junit and iteration continued; if transient GCS or network issues are common, consider logging the error and skipping that URL (or aggregating errors) instead of failing the entire call.
- checkJunitFuncTestXMLExists hardcodes the prow-to-GCS URL replacement logic; consider extracting the prefix constants or sharing existing helpers so future changes to prow or storage paths don’t require updating this function in isolation.

## Individual Comments

### Comment 1
<location path="pkg/ci-failures/main.go" line_range="33" />
<code_context>
-// It then returns only the URLs for which the junit.functest.xml artifact does not exist.
+// ShowCIFailureJobs fetches URLs for the CI failure runs from the data of the latest run.
+// It returns only those URLs for which the junit.functest.xml artifact does not exist.
 func ShowCIFailureJobs() ([]string, error) {
 	// 1. Read and parse the JSON file
 	jsonFile, err := os.Open("./output/kubevirt/kubevirt/results.json")
</code_context>
<issue_to_address>
**question (bug_risk):** Removing the sort changes the determinism of the returned URL order.

Without `JobFailureData` and `sort.Slice`, the URLs now follow whatever ordering `FailedJobLeaderBoard` and its `FailureURLs` happen to use. If callers (including people comparing runs) rely on stable ordering, this may introduce noisy diffs or inconsistent behavior. If we still want determinism but no longer need `sig/id`, consider sorting directly by URL or another stable key.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

// It then returns only the URLs for which the junit.functest.xml artifact does not exist.
// ShowCIFailureJobs fetches URLs for the CI failure runs from the data of the latest run.
// It returns only those URLs for which the junit.functest.xml artifact does not exist.
func ShowCIFailureJobs() ([]string, error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (bug_risk): Removing the sort changes the determinism of the returned URL order.

Without JobFailureData and sort.Slice, the URLs now follow whatever ordering FailedJobLeaderBoard and its FailureURLs happen to use. If callers (including people comparing runs) rely on stable ordering, this may introduce noisy diffs or inconsistent behavior. If we still want determinism but no longer need sig/id, consider sorting directly by URL or another stable key.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2026
The regular expression silently filtered some urls that weren't matching
(i.e. pull-kubevirt-e2e-kind-sriov) was not matching.

Since we basically don't need the sorting it's removed, now we are
directly checking whether the junit is present.

Also any HEAD error was treated as a non-existing junit xml which might
cover some errors - these are now returned.

Finally min and max are removed in favor of the built-ins.

Signed-off-by: Daniel Hiller <dhiller@redhat.com>
@dhiller dhiller force-pushed the fix-filtering-ci-report branch from 4137d34 to 99ea87a Compare March 10, 2026 09:19
@kubevirt-bot kubevirt-bot added size/M and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L labels Mar 10, 2026
@dhiller
Copy link
Contributor Author

dhiller commented Mar 10, 2026

@dollierp rebased - PTAL, thank you !

Addresses a review comment, where it was mentioned that we might retry
the http HEAD in certain occasions. Thus the logic was broadened and now
also supports http.Head requests.

Signed-off-by: Daniel Hiller <dhiller@redhat.com>
@dhiller dhiller requested a review from dollierp March 10, 2026 11:59
@dhiller
Copy link
Contributor Author

dhiller commented Mar 10, 2026

@dollierp addressed your comments, PTAL 🙏

Copy link

@dollierp dollierp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@dhiller dhiller merged commit 5ef02db into kubevirt:main Mar 10, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants