feat(stats): filter unsupported release branches#98
Merged
brianmcarey merged 1 commit intokubevirt:mainfrom Oct 7, 2025
Merged
feat(stats): filter unsupported release branches#98brianmcarey merged 1 commit intokubevirt:mainfrom
brianmcarey merged 1 commit intokubevirt:mainfrom
Conversation
kubevirt-bot
added
the
dco-signoff: yes
Collaborator
|
@dhiller: GitHub didn't allow me to request PR reviews from the following users: cuiyingd. Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
kubevirt-bot
added
needs-rebase
Co-authored-by: Gemini <gemini@google.com> Signed-off-by: Daniel Hiller <dhiller@redhat.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In GetSupportedBranches, consider using a proper semver parser (e.g. github.com/blang/semver) instead of naive string comparison to reliably pick the top 3 release branches.
- The getJobTargetBranch logic is brittle—using regex to extract the “release-X.Y” prefix (or a semantic version parser) will be more robust than splitting on dashes and float-parsing the last segment.
- FilterJobsPerSigs is now doing both SIG metrics counting and branch filtering; consider renaming it (e.g. filterJobsByBranch) or splitting that responsibility for clearer intent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In GetSupportedBranches, consider using a proper semver parser (e.g. github.com/blang/semver) instead of naive string comparison to reliably pick the top 3 release branches.
- The getJobTargetBranch logic is brittle—using regex to extract the “release-X.Y” prefix (or a semantic version parser) will be more robust than splitting on dashes and float-parsing the last segment.
- FilterJobsPerSigs is now doing both SIG metrics counting and branch filtering; consider renaming it (e.g. filterJobsByBranch) or splitting that responsibility for clearer intent.
## Individual Comments
### Comment 1
<location> `pkg/sigretests/main.go:286` </location>
<code_context>
for _, job := range jobs {
+ targetBranch := getJobTargetBranch(job.jobName)
+ isSupported := false
+ for _, supportedBranch := range supportedBranches {
+ if targetBranch == supportedBranch {
+ isSupported = true
</code_context>
<issue_to_address>
**issue (complexity):** Consider using a pre-built set for supportedBranches and a regex in getJobTargetBranch to improve lookup efficiency and code clarity.
Consider building a set for supportedBranches (up front, once) for O(1) lookups and simplifying getJobTargetBranch with a small regex. For example:
```go
// at package scope – compile once
var releaseSuffix = regexp.MustCompile(`-(\d+(\.\d+)?)$`)
func getJobTargetBranch(jobName string) string {
if m := releaseSuffix.FindStringSubmatch(jobName); m != nil {
return "release-" + m[1]
}
return "main"
}
```
And in FilterJobsPerSigs, build your set before the loop:
```go
func FilterJobsPerSigs(jobs []job, supportedBranches []string) SigRetests {
branchSet := make(map[string]struct{}, len(supportedBranches))
for _, b := range supportedBranches {
branchSet[b] = struct{}{}
}
var prSigRetests SigRetests
for _, job := range jobs {
target := getJobTargetBranch(job.jobName)
if _, ok := branchSet[target]; !ok {
continue
}
switch {
case checkSIGCIFailure(job):
prSigRetests.SigCIFailure++
// ... rest of your cases
}
prSigRetests = sortJobNamesOnResult(job, prSigRetests)
}
return prSigRetests
}
```
This removes the inner slice loop and avoids `strconv.ParseFloat` on each job.
</issue_to_address>
### Comment 2
<location> `pkg/gh/main.go:141` </location>
<code_context>
return mergedQuery.Search.Nodes, err
}
+
+func (c *Client) GetSupportedBranches(ctx context.Context) ([]string, error) {
+ var query struct {
+ Repository struct {
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving filtering, sorting, and truncation logic out of the main function into GraphQL or a helper to reduce cognitive overhead.
```suggestion
The manual fetch–filter–sort–truncate in one function is adding cognitive overhead. You can push most of that logic into GraphQL (filter + ordering + limit) or extract a small helper that uses a semver library. For example, with GraphQL ordering + limit you could simplify to:
```go
func (c *Client) GetSupportedBranches(ctx context.Context) ([]string, error) {
var q struct {
Repository struct {
Refs struct {
Nodes []struct{ Name string }
} `graphql:"refs(
refPrefix: \"refs/heads/release-\",
first: 3,
orderBy: {field: ALPHABETICAL, direction: DESC}
)"`
} `graphql:"repository(owner: \"kubevirt\", name: \"kubevirt\")"`
}
if err := c.inner.Query(ctx, &q, nil); err != nil {
return nil, err
}
// Prepend "main" and return exactly the 3 newest release-* branches
out := []string{"main"}
for _, node := range q.Repository.Refs.Nodes {
out = append(out, node.Name)
}
return out, nil
}
```
Or, if you need true semver sort, extract into a helper with golang.org/x/mod/semver:
```go
// helper.go
func latestReleases(names []string, n int) []string {
vers := make([]string, 0, len(names))
for _, b := range names {
vs := strings.TrimPrefix(b, "release-")
vers = append(vers, semver.Canonical("v"+vs))
}
sort.Slice(vers, func(i, j int) bool {
return semver.Compare(vers[i], vers[j]) > 0
})
out := make([]string, 0, min(n, len(vers)))
for i := 0; i < n && i < len(vers); i++ {
out = append(out, "release-"+vers[i][1:]) // drop leading "v"
}
return out
}
// main function
func (c *Client) GetSupportedBranches(ctx context.Context) ([]string, error) {
// fetch all refs/heads/…
// filter for release-*, then:
releases := latestReleases(allBranches, 3)
return append([]string{"main"}, releases...), nil
}
```
Either approach keeps the core function small and moves the sorting/truncation into its own, well-scoped piece.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return mergedQuery.Search.Nodes, err | ||
| } | ||
|
|
||
| func (c *Client) GetSupportedBranches(ctx context.Context) ([]string, error) { |
There was a problem hiding this comment.
issue (complexity): Consider moving filtering, sorting, and truncation logic out of the main function into GraphQL or a helper to reduce cognitive overhead.
Suggested change
| func (c *Client) GetSupportedBranches(ctx context.Context) ([]string, error) { | |
| The manual fetch–filter–sort–truncate in one function is adding cognitive overhead. You can push most of that logic into GraphQL (filter + ordering + limit) or extract a small helper that uses a semver library. For example, with GraphQL ordering + limit you could simplify to: | |
| ```go | |
| func (c *Client) GetSupportedBranches(ctx context.Context) ([]string, error) { | |
| var q struct { | |
| Repository struct { | |
| Refs struct { | |
| Nodes []struct{ Name string } | |
| } `graphql:"refs( | |
| refPrefix: \"refs/heads/release-\", | |
| first: 3, | |
| orderBy: {field: ALPHABETICAL, direction: DESC} | |
| )"` | |
| } `graphql:"repository(owner: \"kubevirt\", name: \"kubevirt\")"` | |
| } | |
| if err := c.inner.Query(ctx, &q, nil); err != nil { | |
| return nil, err | |
| } | |
| // Prepend "main" and return exactly the 3 newest release-* branches | |
| out := []string{"main"} | |
| for _, node := range q.Repository.Refs.Nodes { | |
| out = append(out, node.Name) | |
| } | |
| return out, nil | |
| } |
Or, if you need true semver sort, extract into a helper with golang.org/x/mod/semver:
// helper.go
func latestReleases(names []string, n int) []string {
vers := make([]string, 0, len(names))
for _, b := range names {
vs := strings.TrimPrefix(b, "release-")
vers = append(vers, semver.Canonical("v"+vs))
}
sort.Slice(vers, func(i, j int) bool {
return semver.Compare(vers[i], vers[j]) > 0
})
out := make([]string, 0, min(n, len(vers)))
for i := 0; i < n && i < len(vers); i++ {
out = append(out, "release-"+vers[i][1:]) // drop leading "v"
}
return out
}
// main function
func (c *Client) GetSupportedBranches(ctx context.Context) ([]string, error) {
// fetch all refs/heads/…
// filter for release-*, then:
releases := latestReleases(allBranches, 3)
return append([]string{"main"}, releases...), nil
}Either approach keeps the core function small and moves the sorting/truncation into its own, well-scoped piece.
dhiller
force-pushed
the
filter-unsupported-release-branches
branch
from
2172df8 to
83c8953
Compare
kubevirt-bot
added
size/L
and removed
needs-rebase
Contributor
Author
|
e2e tests do succeed locally: |
brianmcarey
approved these changes
Oct 7, 2025
Member
brianmcarey
left a comment
brianmcarey
left a comment
There was a problem hiding this comment.
/approve
thanks @dhiller !
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
3 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.
What this PR does / why we need it:
Also the k8s-support matrix shows the EOL'd versions: https://github.com/kubevirt/sig-release/blob/main/releases/k8s-support-matrix.md
This effectively means that the KubeVirt community will only actively support the three latest release branches, since only these target supported Kubernetes versions - all other branches are supported at best-effort.
Therefore we should calculate the metrics only considering the supported branches.
This change filters unsupported release branches from the metric calculation process.
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 @brianmcarey @cuiyingd