Switch ran eco gotests version comparison to semver CNF-20904#1322
Open
bazem8 wants to merge 2 commits intorh-ecosystem-edge:mainfrom
Open
Switch ran eco gotests version comparison to semver CNF-20904#1322bazem8 wants to merge 2 commits intorh-ecosystem-edge:mainfrom
bazem8 wants to merge 2 commits intorh-ecosystem-edge:mainfrom
Conversation
Replace IsVersionStringInRange logic that used a regex on the first two dot-separated integers and compared major/minor independently. Add semver_range.go using github.com/Masterminds/semver/v3 for SemVer 2.0 ordering, document X.Y.0-0 lower bounds for pre-releases, and interpret two-segment maximum strings as whole minor lines (upper bound is the next minor). - Add semver_range.go; keep cluster/ZTP helpers in version.go unchanged - Update version_test.go with pre-release and v-prefix cases; document UNIT_TEST=true for local tests without kubeconfig - Declare Masterminds semver/v3 as a direct dependency in go.mod - Update RAN call sites (GitOps/ZTP, TALM, PTP, power management) to pass 4.N.0-0 minimums where the gate means at least OpenShift minor 4.N Made-with: Cursor
- Document three-segment maximum (exclusive next patch) and legacy (true,nil) for unparseable version with empty maximum. - version tests: use ErrorContains instead of comparing errors; add cases for three-segment max; drop testify/require (not vendored). - TALM tests: align By/Skip wording with 4.11.0-0 threshold. Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughThis change refactors the semantic version range checking utility and updates version gating checks across test files. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cnf/ran/internal/version/version_test.go (1)
128-138: Add named subtests to make failures easier to pinpoint.Current table loop makes failures harder to triage. A
namefield +t.Runwould improve diagnostics without changing behavior.♻️ Suggested refactor
func TestIsVersionStringInRange(t *testing.T) { testCases := []struct { + name string version string minimum string maximum string expectedResult bool wantErrSubstring string // if non-empty, err must contain this substring; if empty, err must be nil }{ { + name: "version within minimum only range", version: "4.16.0", minimum: "4.10.0-0", maximum: "", expectedResult: true, }, + // ...populate name for remaining cases... } for _, testCase := range testCases { - result, err := IsVersionStringInRange(testCase.version, testCase.minimum, testCase.maximum) - - assert.Equal(t, testCase.expectedResult, result) - if testCase.wantErrSubstring != "" { - assert.Error(t, err) - assert.ErrorContains(t, err, testCase.wantErrSubstring) - } else { - assert.NoError(t, err) - } + t.Run(testCase.name, func(t *testing.T) { + result, err := IsVersionStringInRange(testCase.version, testCase.minimum, testCase.maximum) + + assert.Equal(t, testCase.expectedResult, result) + if testCase.wantErrSubstring != "" { + assert.Error(t, err) + assert.ErrorContains(t, err, testCase.wantErrSubstring) + } else { + assert.NoError(t, err) + } + }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cnf/ran/internal/version/version_test.go` around lines 128 - 138, The table-driven tests loop over testCases without named subtests, making failures hard to trace; add a name field to the testCase struct and wrap each iteration in t.Run(testCase.name, func(t *testing.T) { ... }) when calling IsVersionStringInRange so assertions run as distinct subtests, keeping the existing assertions and error checks unchanged and referencing the existing testCases slice and IsVersionStringInRange call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cnf/ran/internal/version/semver_range.go`:
- Around line 82-98: The two-segment branch in the function that parses maximums
(after splitSemverCore(s) into core, tail and parts) currently ignores tail and
widens ranges; update the len(parts) == 2 case in that function to check if tail
is non-empty and, if so, return the same invalid-maximum error (e.g.,
fmt.Errorf("invalid maximum provided: '%s'", maximum)) instead of proceeding to
construct a broadened version with semver.NewVersion; keep the existing behavior
for plain "X.Y" (tail empty) and only allow constructing the X.Y.0-0 version
when tail == "".
---
Nitpick comments:
In `@tests/cnf/ran/internal/version/version_test.go`:
- Around line 128-138: The table-driven tests loop over testCases without named
subtests, making failures hard to trace; add a name field to the testCase struct
and wrap each iteration in t.Run(testCase.name, func(t *testing.T) { ... }) when
calling IsVersionStringInRange so assertions run as distinct subtests, keeping
the existing assertions and error checks unchanged and referencing the existing
testCases slice and IsVersionStringInRange call.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02fc7a03-1f9f-49d4-806c-da05688da92d
📒 Files selected for processing (27)
go.modtests/cnf/ran/gitopsztp/tests/ztp-argocd-acm-crs.gotests/cnf/ran/gitopsztp/tests/ztp-argocd-clusters-app.gotests/cnf/ran/gitopsztp/tests/ztp-argocd-hub-templating.gotests/cnf/ran/gitopsztp/tests/ztp-argocd-node-delete.gotests/cnf/ran/gitopsztp/tests/ztp-argocd-policies-app.gotests/cnf/ran/gitopsztp/tests/ztp-bios-day-zero.gotests/cnf/ran/gitopsztp/tests/ztp-cluster-instance-delete.gotests/cnf/ran/gitopsztp/tests/ztp-siteconfig-day-two.gotests/cnf/ran/gitopsztp/tests/ztp-siteconfig-failover.gotests/cnf/ran/gitopsztp/tests/ztp-siteconfig-negative.gotests/cnf/ran/internal/version/semver_range.gotests/cnf/ran/internal/version/version.gotests/cnf/ran/internal/version/version_test.gotests/cnf/ran/powermanagement/tests/cpufreq.gotests/cnf/ran/ptp/internal/consumer/bothversions.gotests/cnf/ran/ptp/internal/mustgather/mustgather.gotests/cnf/ran/ptp/tests/ptp-event-consumer.gotests/cnf/ran/ptp/tests/ptp-interfaces.gotests/cnf/ran/ptp/tests/ptp-log-reduction.gotests/cnf/ran/ptp/tests/ptp-ntp-fallback.gotests/cnf/ran/ptp/tests/ptp-oc-2-port.gotests/cnf/ran/ptp/tests/ptp-process-restart.gotests/cnf/ran/talm/tests/talm-backup.gotests/cnf/ran/talm/tests/talm-batching.gotests/cnf/ran/talm/tests/talm-blockingcr.gotests/cnf/ran/talm/tests/talm-precache.go
💤 Files with no reviewable changes (1)
- tests/cnf/ran/internal/version/version.go
Comment on lines
+82
to
+98
| core, tail := splitSemverCore(s) | ||
| parts := strings.Split(core, ".") | ||
| for _, p := range parts { | ||
| if _, err := strconv.ParseUint(p, 10, 64); err != nil { | ||
| return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum) | ||
| } | ||
| } | ||
|
|
||
| switch len(parts) { | ||
| case 0, 1: | ||
| return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum) | ||
| case 2: | ||
| maj, _ := strconv.ParseUint(parts[0], 10, 64) | ||
| min, _ := strconv.ParseUint(parts[1], 10, 64) | ||
|
|
||
| return semver.NewVersion(fmt.Sprintf("%d.%d.0-0", maj, min+1)) | ||
| default: |
Contributor
There was a problem hiding this comment.
Reject suffixed two-segment maximums instead of silently broadening range.
In the len(parts) == 2 path, tail is currently ignored, so inputs like 4.15-rc.1 or 4.15+meta are treated as plain 4.15. That can admit versions the caller did not intend.
🐛 Proposed fix
func parseMaximumExclusiveUpper(maximum string) (*semver.Version, error) {
if maximum == "" {
return nil, nil
}
s := trimSemverVPrefix(maximum)
core, tail := splitSemverCore(s)
parts := strings.Split(core, ".")
for _, p := range parts {
if _, err := strconv.ParseUint(p, 10, 64); err != nil {
return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum)
}
}
switch len(parts) {
case 0, 1:
return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum)
case 2:
+ if tail != "" {
+ return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum)
+ }
maj, _ := strconv.ParseUint(parts[0], 10, 64)
min, _ := strconv.ParseUint(parts[1], 10, 64)
- return semver.NewVersion(fmt.Sprintf("%d.%d.0-0", maj, min+1))
+ upper, err := semver.NewVersion(fmt.Sprintf("%d.%d.0-0", maj, min+1))
+ if err != nil {
+ return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum)
+ }
+ return upper, nil
default:
full := core
if tail != "" {
full = core + tail
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| core, tail := splitSemverCore(s) | |
| parts := strings.Split(core, ".") | |
| for _, p := range parts { | |
| if _, err := strconv.ParseUint(p, 10, 64); err != nil { | |
| return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum) | |
| } | |
| } | |
| switch len(parts) { | |
| case 0, 1: | |
| return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum) | |
| case 2: | |
| maj, _ := strconv.ParseUint(parts[0], 10, 64) | |
| min, _ := strconv.ParseUint(parts[1], 10, 64) | |
| return semver.NewVersion(fmt.Sprintf("%d.%d.0-0", maj, min+1)) | |
| default: | |
| core, tail := splitSemverCore(s) | |
| parts := strings.Split(core, ".") | |
| for _, p := range parts { | |
| if _, err := strconv.ParseUint(p, 10, 64); err != nil { | |
| return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum) | |
| } | |
| } | |
| switch len(parts) { | |
| case 0, 1: | |
| return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum) | |
| case 2: | |
| if tail != "" { | |
| return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum) | |
| } | |
| maj, _ := strconv.ParseUint(parts[0], 10, 64) | |
| min, _ := strconv.ParseUint(parts[1], 10, 64) | |
| upper, err := semver.NewVersion(fmt.Sprintf("%d.%d.0-0", maj, min+1)) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid maximum provided: '%s'", maximum) | |
| } | |
| return upper, nil | |
| default: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cnf/ran/internal/version/semver_range.go` around lines 82 - 98, The
two-segment branch in the function that parses maximums (after
splitSemverCore(s) into core, tail and parts) currently ignores tail and widens
ranges; update the len(parts) == 2 case in that function to check if tail is
non-empty and, if so, return the same invalid-maximum error (e.g.,
fmt.Errorf("invalid maximum provided: '%s'", maximum)) instead of proceeding to
construct a broadened version with semver.NewVersion; keep the existing behavior
for plain "X.Y" (tail empty) and only allow constructing the X.Y.0-0 version
when tail == "".
klaskosk
reviewed
Apr 15, 2026
Collaborator
klaskosk
left a comment
klaskosk
left a comment
There was a problem hiding this comment.
I suspect it'll be hard to avoid merge conflicts with a PR this big, but those also need fixed before merging. 1 main comments about how we handle maximums, I think it'll simplify the process there. Otherwise just minor things
| @@ -0,0 +1,148 @@ | |||
| package version | |||
Collaborator
There was a problem hiding this comment.
minor point, but since we're just leaving a single function in the old file, we can just move all this content to the old file instead of having a separate new file
| "github.com/Masterminds/semver/v3" | ||
| ) | ||
|
|
||
| // IsVersionStringInRange reports whether version satisfies minimum <= version < maximumUpper using SemVer 2.0 |
Collaborator
There was a problem hiding this comment.
I think we can simplify the maximum handling by making it exclusive. Similar to how slicing an array like arr[1:3] means grab elements arr[1] and arr[2] but not arr[3], we could do the same thing with versions. The math term would be a "half-open interval"
The reason this is nicer is that there's a clear minimum for a given y-stream (x.y.0-0), but there's no clear maximum
So if we wanted to do all z-streams for 4.16 and 4.17, we could write the function call as IsVersionStringInRange(myVersionString, "4.16.0-0", "4.18.0-0") and then make sure the parsed myVersion is greater than or equal to (equivalently, not less than) 4.16.0-0 and less than 4.18.0-0
| k8s.io/apiextensions-apiserver v0.34.5 | ||
| github.com/Masterminds/semver/v3 v3.4.0 | ||
| github.com/rh-ecosystem-edge/eco-goinfra v0.0.0-20260324223608-9209edd329fe | ||
| ) |
Collaborator
There was a problem hiding this comment.
nit: you can add the github.com/Masterminds/semver/v3 v3.4.0 line to the top require block
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.
switch RAN eco-gotests version comparison to semver - CNF-20904
passed the review of "/cnf-ran-review" .
Summary by CodeRabbit
Release Notes
Refactor
Chores