OCP5 enablement in ci-tools #4898
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughIntroduce a centralized version utility (parsing, validation, prior-version resolution), propagate it across CLIs and packages to support releases >=4.x (including 5.x), expand tests, and adapt callers to explicitly obtain and handle past/past-past versions with error handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmguzik 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
| if name[0] >= '4' && name[0] <= '9' && name[1] == '.' { | ||
| return true | ||
| } |
There was a problem hiding this comment.
With the major release changing frequently now, we could quickly reach openshift 10.0, right?
There was a problem hiding this comment.
not that quick imo, I did not want to complicate situation with version 10 for now.
openshift-merge-robot
added
the
needs-rebase
jmguzik
force-pushed
the
ocp5
branch
2 times, most recently
from
afeca20 to
19d9423
Compare
openshift-merge-robot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/api/leases.go`:
- Around line 71-93: The parseVersionFromBranch function currently only checks
"openshift-" and "release-" prefixes, causing enterprise-* branches to be
rejected (which breaks branchValidForIPPoolLease). Update the prefixes slice
inside parseVersionFromBranch to also include "enterprise-" (i.e., prefixes :=
[]string{"openshift-", "release-", "enterprise-"}) so enterprise-X.Y branch
names like "enterprise-3.11" parse correctly and return ok=true; no other logic
changes required.
In `@pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go`:
- Around line 237-253: Before calling api.GetAllPreviousVersionsSimple,
validate/normalize job.FromRelease.StringVal and job.Release to ensure they are
in X.Y format (ParseVersion requires exactly two dot-separated components).
Locate the blocks referencing job.FromRelease.StringVal and job.Release in
pass_fail.go and either (a) normalize values by extracting the first two numeric
dot components (e.g., split on non-digit/dot or on '.' and join the first two
elements) and pass the normalized "X.Y" to GetAllPreviousVersionsSimple, or (b)
validate with a regex like ^\d+\.\d+$ and if validation fails, log a clear error
via fmt.Printf (including the original value and reason) and avoid silently
skipping fallback. Ensure both the GetAllPreviousVersionsSimple calls and their
error messages reference the validated/normalized strings so failures are
visible.
🧹 Nitpick comments (2)
pkg/release/prerelease/client.go (1)
22-33: Consider adding input validation for robustness.The function correctly extracts the major version from bounds and constructs a stream name. However, it assumes
parts[0]will always be a valid major version number. If version bounds unexpectedly contain malformed data (e.g.,"v5.0","latest"), the function would return streams like"v5-stable"or"latest-stable".Given that the fallback to
"4-stable"exists and the VersionBounds API contract likely enforces proper format, this may be acceptable. If this code path becomes critical, consider validating thatparts[0]is numeric.cmd/branchingconfigmanagers/tide-config-manager/main.go (1)
502-534: Consider usingstrconv.Atoifor cleaner parsing.The manual character-by-character parsing works correctly, but
strconv.Atoi(already used elsewhere in this PR, e.g.,cmd/cvp-trigger/main.go) would simplify this code and improve readability.♻️ Suggested simplification
func isValidVersion(version string) bool { parts := strings.Split(version, ".") if len(parts) != 2 { return false } - // Validate major version (must be 4 or higher) - major := parts[0] - if major == "" { - return false - } - for _, char := range major { - if char < '0' || char > '9' { - return false - } - } - majorNum := 0 - for _, char := range major { - majorNum = majorNum*10 + int(char-'0') - } - if majorNum < 4 { + majorNum, err := strconv.Atoi(parts[0]) + if err != nil || majorNum < 4 { return false } - // Validate minor version - minor := parts[1] - if !isValidMinorVersion(minor) { - return false - } - - return true + _, err = strconv.Atoi(parts[1]) + return err == nil }This would also require adding
"strconv"to the imports.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (18)
cmd/branchingconfigmanagers/tide-config-manager/main.gocmd/branchingconfigmanagers/tide-config-manager/main_test.gocmd/ci-operator-config-mirror/main.gocmd/cvp-trigger/main.gocmd/ocp-build-data-enforcer/main.gocmd/registry-replacer/main.gopkg/api/leases.gopkg/api/leases_test.gopkg/api/metadata.gopkg/api/metadata_test.gopkg/api/ocplifecycle/ocplifecycle.gopkg/api/version.gopkg/api/version_test.gopkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.gopkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.gopkg/rehearse/jobs.gopkg/release/prerelease/client.gopkg/steps/release/promote.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/ci-operator-config-mirror/main.go
- cmd/branchingconfigmanagers/tide-config-manager/main_test.go
- cmd/ocp-build-data-enforcer/main.go
- pkg/api/version.go
- pkg/api/metadata_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/api/metadata.gopkg/api/leases_test.gopkg/api/version_test.gocmd/branchingconfigmanagers/tide-config-manager/main.gopkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.gocmd/cvp-trigger/main.gocmd/registry-replacer/main.gopkg/api/ocplifecycle/ocplifecycle.gopkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.gopkg/api/leases.gopkg/release/prerelease/client.gopkg/rehearse/jobs.gopkg/steps/release/promote.go
🧬 Code graph analysis (5)
pkg/api/version_test.go (1)
pkg/api/version.go (6)
GetPreviousVersion(47-87)ParsedVersion(17-20)ParseVersion(26-43)GetPreviousVersionSimple(135-150)GetAllPreviousVersions(91-131)GetAllPreviousVersionsSimple(153-168)
pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go (3)
pkg/jobrunaggregator/jobrunaggregatorapi/types_row_job.go (1)
JobRowWithVariants(17-30)pkg/api/version.go (1)
GetAllPreviousVersionsSimple(153-168)pkg/api/types.go (1)
Release(452-466)
pkg/api/ocplifecycle/ocplifecycle.go (1)
pkg/api/version.go (1)
GetPreviousVersionSimple(135-150)
pkg/release/prerelease/client.go (2)
pkg/api/types.go (4)
VersionBounds(380-386)ReleaseDescriptor(350-361)ReleaseProductOCP(408-408)ReleaseArchitectureAMD64(417-417)pkg/release/candidate/client.go (2)
Endpoint(41-43)ResolveReleaseCommon(75-115)
pkg/steps/release/promote.go (1)
pkg/release/prerelease/client.go (1)
StableLatestMajorMinor(61-71)
🔇 Additional comments (26)
pkg/rehearse/jobs.go (2)
56-68: LGTM!The constants and variable declarations are well-structured. Using a
varfornumMinorVersionsallows test flexibility while keeping version bounds as constants. The relevancy scheme correctly prioritizesmaster/main(10000) over all release branches, with newer major.minor versions receiving higher relevancy values.
70-79: LGTM!The init logic correctly generates relevancy values for all major.minor combinations across both
release-X.Yandopenshift-X.Ybranch naming conventions. The formula ensures proper ordering: higher major versions always outrank lower ones (100-point blocks), and within a major, higher minors are more relevant.pkg/api/ocplifecycle/ocplifecycle.go (3)
16-16: LGTM!Import updated to reference the
apipackage directly, enabling use of the centralized version utilities.
213-221: LGTM!Clean delegation to centralized version logic. The silent error handling (returning
"") is appropriate for this utility method since callers check for empty strings.
223-234: LGTM!Correctly chains version resolution to get N-2 version, properly handling cross-major transitions (e.g.,
5.1→5.0→4.22).pkg/steps/release/promote.go (2)
225-233: LGTM - Clean implementation for multi-stream version resolution.The descending iteration from 9-stable to 4-stable is a sensible approach for finding the latest available CLI version. The silent error swallowing per-stream is appropriate here since we're using a fallback pattern.
One minor observation: individual errors are discarded, which could make debugging harder if all streams fail unexpectedly. Consider logging at debug level if debugging becomes an issue in the future.
108-112: LGTM - Updated fallback and new helper usage.The fallback version update from 4.14 to 4.20 aligns with the PR's goal of supporting newer OpenShift versions. The error handling with warn-level logging and graceful degradation is appropriate.
pkg/release/prerelease/client.go (2)
51-58: LGTM - Clean generalization of stream-based version resolution.The refactoring to accept a stream parameter is well-done:
stableLatestnow accepts any stream name, enabling the caller to specify different stable streamsStableLatestMajorMinorsignature update properly propagates the stream parameter- The typo fix ("sematic" → "semantic") is a good cleanup
Also applies to: 60-70
14-20: LGTM - Proper integration of dynamic stream derivation.The change correctly delegates to
deriveStreamFromBoundswhen no explicit stream is configured, maintaining backward compatibility with the"4-stable"default while enabling dynamic stream selection based on version bounds.pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.go (2)
14-14: LGTM!Import correctly added to support the centralized version parsing and handling utilities.
100-130: Clean refactoring to generalized version handling.The logic correctly:
- Filters for versions with Major >= 4
- Sorts by Major then Minor descending to get the highest version
- Uses centralized
GetPreviousVersionfor determining the previous release- Includes proper error handling
One consideration:
parsedVersions[0].String()is used forcurrent, whilevalidVersionscontains the original API strings. Verify thatapi.GetPreviousVersionperforms semantic version comparison rather than exact string matching, to avoid issues ifString()produces a slightly different format.cmd/cvp-trigger/main.go (3)
11-11: LGTM!The
strconvimport is correctly added to support the numeric parsing in the newisValidOCPVersionfunction.
157-160: LGTM!The validation logic correctly uses the new
isValidOCPVersionfunction and provides a clear, actionable error message with format requirements and examples.
184-202: LGTM!The
isValidOCPVersionfunction correctly validates the version format:
- Ensures exactly two parts separated by "."
- Validates both major and minor are numeric
- Enforces major version >= 4
This aligns well with the centralized version validation patterns being introduced across the codebase.
pkg/api/metadata.go (2)
114-114: LGTM!The regex correctly extends support from 4.x-only to 4.x–9.x branches. The pattern
[4-9]\.[0-9]+properly matches major versions 4–9 with any minor version.
120-121: LGTM!The updated logic correctly extracts the full version string (e.g., "5.0") from capture group 2, which is used as the flavor for config map naming.
pkg/api/leases.go (2)
45-48: LGTM!Good use of named constants with clear documentation explaining the minimum IP pool version requirement.
53-68: LGTM!The logic correctly handles:
master/mainbranches as always valid- Major versions > 4 (5.x+) as always valid
- Major version 4 requiring minor >= 16
This properly enables IP pools for all 5.x+ branches while maintaining the 4.16+ requirement for 4.x.
pkg/api/leases_test.go (1)
87-116: LGTM!Comprehensive test coverage for 5.x branches:
- Covers both
release-andopenshift-prefixes- Tests 5.0 and 5.1 to verify minor version handling
- Expected lease configuration is consistent with existing 4.16+ test cases
cmd/branchingconfigmanagers/tide-config-manager/main.go (2)
487-500: LGTM!The function correctly identifies versioned branches by checking both
release-andopenshift-prefixes and delegating version validation toisValidVersion.
536-549: LGTM!The function correctly validates that a minor version string is non-empty and contains only numeric characters.
pkg/api/version_test.go (4)
43-47: Reconfirm 4.1 → 4.0 expectations against the “4.0 never existed” rule.The PR objective mentions that 4.0 never existed; if the intent is to jump from 4.1 directly to the last 3.x via
VersionTransitionOverrides, these expectations should not assert4.0. Please verify and align tests/overrides accordingly.Also applies to: 167-170
105-137: Solid coverage for ParseVersion edge cases.
202-261: Good coverage for GetAllPreviousVersions scenarios.
263-315: LGTM for GetAllPreviousVersionsSimple.cmd/registry-replacer/main.go (1)
68-98: Explicit major flag + validation is a good hardening.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/ocp-build-data-enforcer/main.go`:
- Around line 41-43: processDockerfile and other upstream-branch selections are
still hard-coded to "release-4.6"; update the logic that chooses the upstream
branch to use the configured flags (o.majorMinor.Major and o.majorMinor.Minor)
instead of the literal "4" and "6". Locate where the branch string is
constructed (references: processDockerfile and the three other branch-selection
sites mentioned) and replace the hard-coded value with a formatted branch name
built from o.majorMinor.Major and o.majorMinor.Minor (e.g.
"release-<major>.<minor>") so fetching and diffs use the selected version.
🧹 Nitpick comments (5)
pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.go (1)
100-124: Data inconsistency after sortingparsedVersions.After sorting
parsedVersionsat lines 117-122, the correspondence betweenparsedVersionsandvalidVersionsis broken. WhileparsedVersions[0].String()is used forcurrent(which is fine),validVersionspassed toapi.GetPreviousVersionremains unsorted and may not match the sorted order.This doesn't affect correctness here since
GetPreviousVersionsearches through allavailableVersionsregardless of order. However, maintaining two parallel lists that fall out of sync is a subtle code smell that could cause issues in future modifications.♻️ Consider using only parsedVersions
- var validVersions []string - var parsedVersions []api.ParsedVersion - for _, d := range sippyRelease.Releases { pv, err := api.ParseVersion(d) if err != nil || pv.Major < 4 { continue } - validVersions = append(validVersions, d) parsedVersions = append(parsedVersions, pv) } if len(parsedVersions) < 1 { return "", "", fmt.Errorf("no releases found") } sort.SliceStable(parsedVersions, func(i, j int) bool { if parsedVersions[i].Major != parsedVersions[j].Major { return parsedVersions[i].Major > parsedVersions[j].Major } return parsedVersions[i].Minor > parsedVersions[j].Minor }) current = parsedVersions[0].String() + // Rebuild validVersions from sorted parsedVersions for consistency + validVersions := make([]string, len(parsedVersions)) + for i, pv := range parsedVersions { + validVersions[i] = pv.String() + } previous, err = api.GetPreviousVersion(current, validVersions)pkg/api/version.go (1)
89-131: Verify intended behavior for non-override cases.The function name
GetAllPreviousVersionssuggests returning multiple versions, but for non-override cases (lines 101-103 and 130), it only returns a single-element slice. This appears intentional based on the commented example at line 14 showing multiple previous versions only in override scenarios.Consider adding a doc comment clarifying this behavior: overrides can specify multiple previous versions (e.g., for upgrade path testing), while natural progression always yields exactly one.
pkg/steps/release/promote.go (1)
225-233: Streaming search approach looks correct; consider logging which stream succeeded.The descending iteration from
9-stableto4-stableis a sensible strategy for finding the latest available CLI. However, all errors are silently discarded, making debugging harder if none of the streams work.Consider logging at debug level when a stream lookup fails, so operators can trace which streams were tried:
💡 Optional improvement
func getLatestStableCLIVersion(client *http.Client) (string, error) { for major := 9; major >= 4; major-- { stream := fmt.Sprintf("%d-stable", major) - if version, err := prerelease.StableLatestMajorMinor(client, stream); err == nil { + version, err := prerelease.StableLatestMajorMinor(client, stream) + if err == nil { + logrus.WithField("stream", stream).Debugf("Found stable CLI version %s", version) return version, nil } + logrus.WithField("stream", stream).WithError(err).Debug("Stream lookup failed, trying next") } return "", fmt.Errorf("no stable CLI version found in any stream (tried 9-stable through 4-stable)") }pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go (2)
257-272: Consider extracting the job-matching logic to reduce duplication.The same matching criteria (Architecture, Topology, Network, Platform, IPMode, and normalized name) are repeated in three separate loops. This could be simplified by extracting a helper function.
♻️ Optional refactor to reduce duplication
func matchesJobVariants(candidate, target *jobrunaggregatorapi.JobRowWithVariants, normalizedName string) bool { return candidate.Architecture == target.Architecture && candidate.Topology == target.Topology && candidate.Network == target.Network && candidate.Platform == target.Platform && candidate.IPMode == target.IPMode && normalizeJobName(candidate.JobName, candidate.FromRelease.StringVal, candidate.Release) == normalizedName }Then use this helper in each loop, only varying the FromRelease/Release comparison.
Also applies to: 275-290, 292-307
257-272: Performance note: nested loops over candidates and all jobs.The nested iteration over
fromReleaseCandidates × toReleaseCandidates × allJobscould become slow ifallJobsis large. However, sinceGetAllPreviousVersionsSimpletypically returns only one candidate (based on the implementation in version.go), and this function is called per job during fallback scenarios, the impact is likely minimal.If profiling reveals this as a hotspot, consider building a lookup map keyed by the variant properties.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (18)
cmd/branchingconfigmanagers/tide-config-manager/main.gocmd/branchingconfigmanagers/tide-config-manager/main_test.gocmd/ci-operator-config-mirror/main.gocmd/cvp-trigger/main.gocmd/ocp-build-data-enforcer/main.gocmd/registry-replacer/main.gopkg/api/leases.gopkg/api/leases_test.gopkg/api/metadata.gopkg/api/metadata_test.gopkg/api/ocplifecycle/ocplifecycle.gopkg/api/version.gopkg/api/version_test.gopkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.gopkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.gopkg/rehearse/jobs.gopkg/release/prerelease/client.gopkg/steps/release/promote.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/leases_test.go
- cmd/branchingconfigmanagers/tide-config-manager/main_test.go
- cmd/cvp-trigger/main.go
- pkg/api/metadata_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/api/metadata.gocmd/branchingconfigmanagers/tide-config-manager/main.gopkg/api/version_test.gocmd/ci-operator-config-mirror/main.gopkg/api/ocplifecycle/ocplifecycle.gopkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.gopkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.gopkg/release/prerelease/client.gocmd/registry-replacer/main.gopkg/api/version.gocmd/ocp-build-data-enforcer/main.gopkg/rehearse/jobs.gopkg/api/leases.gopkg/steps/release/promote.go
🧬 Code graph analysis (5)
pkg/api/ocplifecycle/ocplifecycle.go (2)
pkg/api/version.go (1)
GetPreviousVersionSimple(135-150)pkg/api/types.go (3)
ReleaseBuildConfiguration(37-140)LatestReleaseName(2865-2865)Candidate(364-370)
pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.go (1)
pkg/api/version.go (3)
ParsedVersion(17-20)ParseVersion(26-43)GetPreviousVersion(47-87)
pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go (3)
pkg/jobrunaggregator/jobrunaggregatorapi/types_row_job.go (1)
JobRowWithVariants(17-30)pkg/api/version.go (1)
GetAllPreviousVersionsSimple(153-168)pkg/api/types.go (1)
Release(452-466)
pkg/release/prerelease/client.go (2)
pkg/api/types.go (4)
VersionBounds(380-386)ReleaseDescriptor(350-361)ReleaseProductOCP(408-408)ReleaseArchitectureAMD64(417-417)pkg/release/candidate/client.go (2)
Endpoint(41-43)ResolveReleaseCommon(75-115)
pkg/steps/release/promote.go (1)
pkg/release/prerelease/client.go (1)
StableLatestMajorMinor(61-71)
🔇 Additional comments (27)
pkg/api/metadata.go (1)
114-121: LGTM!The regex update correctly broadens branch matching from 4.x-only to 4.x–9.x while maintaining backward compatibility. The capture group usage (
m[2]) correctly extracts the fullmajor.minorversion string for the flavor.cmd/ci-operator-config-mirror/main.go (1)
268-277: LGTM: integration stream detection generalized to 4–9.cmd/registry-replacer/main.go (1)
68-69: LGTM: explicit current-release-major flag and validation.Also applies to: 93-98
cmd/branchingconfigmanagers/tide-config-manager/main.go (1)
487-549: LGTM: generalized versioned-branch detection.pkg/rehearse/jobs.go (1)
56-77: LGTM: relevancy now covers majors 4–9.pkg/api/leases.go (2)
44-69: LGTM! Clean version validation logic.The constants and branching logic correctly handle the IP pool lease validation for OCP 4.16+ and all 5.x+ versions. The separation of version parsing into a dedicated helper improves maintainability.
71-94: LGTM! Well-structured branch parsing helper.The function properly handles multiple prefix formats and validates the version components. Using
continueto try alternative prefixes when parsing fails is a good approach.pkg/release/prerelease/client.go (3)
51-58: LGTM! Good refactoring for stream flexibility.The function now properly accepts the stream parameter, enabling support for different major version streams (4-stable, 5-stable, etc.).
60-71: LGTM! Signature update and typo fix.Good update to propagate the stream parameter and fix the typo in the error message.
22-33: No issue found. The function correctly handles both single-digit and semantic versioning formats identically. Whetherbounds.Lowerorbounds.Uppercontains "5" or "5.0.0",strings.Split()extracts the major version correctly and returns "5-stable" in both cases. All test data uses semantic versioning, and validation ensures bounds are non-empty; the function's behavior is sound.pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.go (1)
125-130: LGTM! Good error handling for previous version determination.Proper error wrapping with context makes debugging easier.
pkg/api/ocplifecycle/ocplifecycle.go (3)
213-221: LGTM! Clean delegation to centralized version logic.The method properly delegates to
api.GetPreviousVersionSimpleand handles errors by returning an empty string, maintaining backward compatibility with existing callers that expect this behavior.
223-234: LGTM! Correctly computes n-2 version.The method properly chains version lookups to get two versions back, with consistent empty-string error handling.
269-278: LGTM! Type reference update to centralized API package.Clean update to use
api.ReleaseBuildConfigurationandapi.LatestReleaseNameinstead of the previous import alias.pkg/api/version.go (4)
10-15: Well-designed transition overrides map.Using a centralized map for cross-major version transitions (like 5.0 → 4.22) provides a single source of truth and makes future updates straightforward. The commented example for 5.1 serves as good documentation.
26-43: LGTM! Clean version parsing with good error messages.The function correctly validates the format and provides helpful error messages that include the problematic input.
45-87: LGTM! Robust previous version determination.The function correctly handles:
- Override lookups for cross-major transitions
- Natural minor version decrement
- Fallback to searching available versions for X.0 cases
The sorting logic correctly selects the highest minor version from the previous major.
133-168: LGTM! Clean simplified variants.These functions correctly handle the case where no available versions list is provided, returning errors for X.0 versions that require override configuration rather than silently failing or making incorrect assumptions.
pkg/steps/release/promote.go (1)
108-112: LGTM on the fallback version update.The change from 4.14 to 4.20 as the default fallback CLI version is appropriate given the OCP5 enablement scope.
pkg/api/version_test.go (5)
43-47: Verify the expected behavior for 4.1's previous version.The test expects
GetPreviousVersion("4.1", ...)to return"4.0". However, the PR objectives state that "4.0 never existed (4.1 was the first 4.x release)."If the function is intended to return a computed previous version regardless of real-world existence, this is fine. But if callers rely on this for actual release lookups, returning "4.0" could cause downstream failures.
Confirm this is the intended behavior and that callers handle non-existent versions appropriately.
166-170: Same concern applies toGetPreviousVersionSimple("4.1").This test also expects
"4.0"as the previous version for4.1. Ensure consistency with the design decision noted above.
7-103: Good test coverage forGetPreviousVersion.The test cases cover:
- Override-based transitions (5.0 → 4.22)
- Natural progression within a major version
- Cross-major fallback (6.0 finding highest 5.x)
- Error cases for invalid formats
This provides solid coverage for the version transition logic.
105-137: LGTM onParseVersiontests.Good coverage of valid inputs, invalid formats, and edge cases like empty strings.
202-315: LGTM onGetAllPreviousVersionsandGetAllPreviousVersionsSimpletests.The tests appropriately verify both the override-based and natural progression behaviors, including error conditions.
pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go (3)
14-14: LGTM on the new import.Adding the
apipackage import to leverage centralized version utilities is the right approach.
230-232: Good nil check before proceeding.Returning early when
jobis not found prevents nil pointer dereferences and is appropriate behavior.
237-253: LGTM on the candidate version retrieval.Using
api.GetAllPreviousVersionsSimplefor bothFromReleaseandReleasecentralizes the version logic and aligns with the PR's goal of generalizing version handling. The error handling with informative logging is appropriate.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| flag.StringVar(&o.ocpBuildDataRepoDir, "ocp-build-data-repo-dir", "../ocp-build-data", "The directory in which the ocp-build-data repository is") | ||
| flag.StringVar(&o.majorMinor.Major, "major", "4", "The major version to target") | ||
| flag.StringVar(&o.majorMinor.Minor, "minor", "6", "The minor version to target") |
There was a problem hiding this comment.
Use the selected major/minor when choosing the upstream branch.
With the new --major/--minor flags, processDockerfile still fetches from the hard-coded release-4.6 (Line 115), so targeting 5.x+ will pull the wrong Dockerfile and produce incorrect diffs/PRs.
Proposed fix
@@
- errGroup := &errgroup.Group{}
+ errGroup := &errgroup.Group{}
+ releaseBranch := fmt.Sprintf("release-%s", opts.majorMinor.String())
for idx := range configs {
idx := idx
errGroup.Go(func() error {
- return processDockerfile(configs[idx], diffProcessor.addDiff)
+ return processDockerfile(configs[idx], diffProcessor.addDiff, releaseBranch)
})
}
@@
-func processDockerfile(config ocpbuilddata.OCPImageConfig, processor diffProcessorFunc) error {
+func processDockerfile(config ocpbuilddata.OCPImageConfig, processor diffProcessorFunc, releaseBranch string) error {
@@
- getter := github.FileGetterFactory(config.PublicRepo.Org, config.PublicRepo.Repo, "release-4.6")
+ getter := github.FileGetterFactory(config.PublicRepo.Org, config.PublicRepo.Repo, releaseBranch)Also applies to: 90-94, 109-116
🤖 Prompt for AI Agents
In `@cmd/ocp-build-data-enforcer/main.go` around lines 41 - 43, processDockerfile
and other upstream-branch selections are still hard-coded to "release-4.6";
update the logic that chooses the upstream branch to use the configured flags
(o.majorMinor.Major and o.majorMinor.Minor) instead of the literal "4" and "6".
Locate where the branch string is constructed (references: processDockerfile and
the three other branch-selection sites mentioned) and replace the hard-coded
value with a formatted branch name built from o.majorMinor.Major and
o.majorMinor.Minor (e.g. "release-<major>.<minor>") so fetching and diffs use
the selected version.
This change prepares ci-tools for OpenShift 5.x support while maintaining backward compatibility with 4.x. It also generalizes version handling to support future major versions (6.x through 9.x). Key changes: Version Transition Logic (pkg/api/version.go): - Added VersionTransitionOverrides map as single source of truth for cross-major version transitions (e.g., 5.0 -> 4.22) - Added GetPreviousVersion and GetPreviousVersionSimple functions - Support for multiple upgrade sources (EUS-style upgrades) Updated Components: - pkg/api/ocplifecycle: GetPastVersion/GetPastPastVersion now use centralized version logic - pkg/jobrunaggregator: pass_fail.go and util.go use new version functions - pkg/rehearse/jobs.go: Generalized relevancy for major versions 4-9 - pkg/release/prerelease: Dynamic stream derivation from version bounds - pkg/steps/release/promote.go: Tries stable streams 9 down to 4 CLI Tools Updated: - cmd/cvp-trigger: Validates versions >= 4.x (not just 4.x) - cmd/registry-replacer: Added --current-release-major flag - cmd/ocp-build-data-enforcer: Added --major flag The repository is now ready for OpenShift versions 4.x through 9.x. Signed-off-by: Jakub Guzik <[email protected]>
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)
cmd/ocp-build-data-enforcer/main.go (1)
304-317: Hard-coded version in PR body documentation link.The ocp-build-data reference link at line 316 is hard-coded to
openshift-4.6. This should be dynamically constructed using themajorMinoroption to match the targeted version.Proposed fix
Pass
majorMinorto the PR creation context and update the link:- "[2]: https://github.com/openshift/ocp-build-data/tree/openshift-4.6/images", + fmt.Sprintf("[2]: https://github.com/openshift/ocp-build-data/tree/openshift-%s/images", majorMinor.String()),This requires threading
majorMinorthrough todiffProcessoror theprocessmethod.
🤖 Fix all issues with AI agents
In `@pkg/api/ocplifecycle/ocplifecycle.go`:
- Around line 213-234: GetPastVersion and GetPastPastVersion silently swallow
errors from api.GetPreviousVersionSimple and ParseMajorMinor, leading to invalid
branch names; change the signatures of MajorMinor.GetPastVersion and
GetPastPastVersion to return (string, error), propagate the error from
api.GetPreviousVersionSimple in GetPastVersion (do not return ""), and in
GetPastPastVersion return an error if GetPastVersion or ParseMajorMinor fail;
update all callers (e.g., GA branch construction) to handle the error rather
than treating an empty string as a valid version so invalid branch names are not
generated.
In `@pkg/api/version_test.go`:
- Around line 43-47: Update the test case named "4.1 computes previous as 4.0
(natural progression)" in pkg/api/version_test.go to expect the last 3.x release
instead of "4.0" (the version-transition rules treat 4.1 as the first 4.x so
previous should fall back to the last 3.x from availableVersions); replace the
expected value that currently equals "4.0" with the appropriate last 3.x entry
from the availableVersions slice (and make the analogous change for the similar
test case around the other occurrence mentioned at lines 167-170) so the
assertions align with the override behavior.
♻️ Duplicate comments (3)
cmd/ocp-build-data-enforcer/main.go (1)
42-43: Flag addition is correct; downstream usage issue already noted.The
--majorflag is properly defined alongside--minor. The past review comment correctly identifies thatprocessDockerfileand the PR body still use hard-codedrelease-4.6references (lines 115 and 316) which need to useopts.majorMinorinstead.pkg/api/leases.go (1)
71-78: Confirm enterprise- branches are supported.*
parseVersionFromBranchignoresenterprise-prefixes. If those branches should be eligible for IP pool leases, they will be rejected. Consider adding the prefix if needed.✅ Suggested adjustment (if enterprise-* branches are valid)
- prefixes := []string{"openshift-", "release-"} + prefixes := []string{"openshift-", "release-", "enterprise-"}Please verify whether
enterprise-*branches appear in configs or tests:#!/bin/bash rg -n 'enterprise-\d+\.\d+' -g '!**/vendor/**'pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go (1)
237-252: Normalize Release/FromRelease before callingGetAllPreviousVersionsSimple.
ParseVersionis strict about X.Y. If values include patch/suffix (e.g.,4.12.0,stable-4.12), this will error and disable fallback. Consider validating/normalizing inputs or surfacing a clearer message before returning.
🧹 Nitpick comments (2)
pkg/release/prerelease/client.go (1)
22-33: Logic is sound but lacks validation for malformed versions.The function correctly extracts the major version and falls back to
"4-stable"per the documented default inapi/types.go. However, it doesn't validate thatparts[0]is numeric—a malformed version like"abc.1"would produce"abc-stable".Given that version bounds are typically well-formed from upstream systems, this is low risk. Consider adding validation if defensive coding is preferred.
🔧 Optional: Add numeric validation
func deriveStreamFromBounds(bounds api.VersionBounds) string { for _, version := range []string{bounds.Lower, bounds.Upper} { if version == "" { continue } parts := strings.Split(version, ".") - if len(parts) >= 1 && parts[0] != "" { + if len(parts) >= 1 && parts[0] != "" && isNumeric(parts[0]) { return fmt.Sprintf("%s-stable", parts[0]) } } return "4-stable" } + +func isNumeric(s string) bool { + for _, c := range s { + if c < '0' || c > '9' { + return false + } + } + return len(s) > 0 +}pkg/rehearse/jobs.go (1)
56-77: Guard against future minor versions exceedingnumMinorVersions.
If a branch likerelease-4.50+appears, it will fall back to relevancy0and sort last. Consider computing relevancy on-demand from parsed versions or documenting/expanding the bound to avoid silent mis-ordering.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (19)
cmd/branchingconfigmanagers/tide-config-manager/main.gocmd/branchingconfigmanagers/tide-config-manager/main_test.gocmd/ci-operator-config-mirror/main.gocmd/cvp-trigger/main.gocmd/ocp-build-data-enforcer/main.gocmd/registry-replacer/main.gopkg/api/leases.gopkg/api/leases_test.gopkg/api/metadata.gopkg/api/metadata_test.gopkg/api/ocplifecycle/ocplifecycle.gopkg/api/version.gopkg/api/version_test.gopkg/api/zz_generated.deepcopy.gopkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.gopkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.gopkg/rehearse/jobs.gopkg/release/prerelease/client.gopkg/steps/release/promote.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/api/metadata.go
- pkg/api/metadata_test.go
- cmd/ci-operator-config-mirror/main.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/api/version_test.gopkg/rehearse/jobs.gocmd/ocp-build-data-enforcer/main.gopkg/api/zz_generated.deepcopy.gocmd/registry-replacer/main.gopkg/release/prerelease/client.gopkg/api/version.gopkg/api/leases.gopkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.gocmd/cvp-trigger/main.gopkg/api/ocplifecycle/ocplifecycle.gopkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.gocmd/branchingconfigmanagers/tide-config-manager/main_test.gopkg/steps/release/promote.gopkg/api/leases_test.gocmd/branchingconfigmanagers/tide-config-manager/main.go
🧬 Code graph analysis (7)
pkg/api/version_test.go (2)
pkg/testhelper/accessory.go (1)
T(51-57)pkg/api/version.go (6)
GetPreviousVersion(47-87)ParsedVersion(17-20)ParseVersion(26-43)GetPreviousVersionSimple(135-150)GetAllPreviousVersions(91-131)GetAllPreviousVersionsSimple(153-168)
pkg/api/zz_generated.deepcopy.go (1)
pkg/api/version.go (1)
ParsedVersion(17-20)
pkg/release/prerelease/client.go (2)
pkg/api/types.go (4)
VersionBounds(380-386)ReleaseDescriptor(350-361)ReleaseProductOCP(408-408)ReleaseArchitectureAMD64(417-417)pkg/release/candidate/client.go (2)
Endpoint(41-43)ResolveReleaseCommon(75-115)
pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.go (1)
pkg/api/version.go (3)
ParsedVersion(17-20)ParseVersion(26-43)GetPreviousVersion(47-87)
pkg/api/ocplifecycle/ocplifecycle.go (2)
pkg/api/version.go (1)
GetPreviousVersionSimple(135-150)pkg/api/types.go (3)
ReleaseBuildConfiguration(37-140)LatestReleaseName(2865-2865)Candidate(364-370)
pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go (3)
pkg/jobrunaggregator/jobrunaggregatorapi/types_row_job.go (1)
JobRowWithVariants(17-30)pkg/api/version.go (1)
GetAllPreviousVersionsSimple(153-168)pkg/api/types.go (1)
Release(452-466)
pkg/api/leases_test.go (2)
pkg/api/types.go (5)
MultiStageTestConfigurationLiteral(1250-1289)ClusterProfile(1334-1334)ClusterProfileAWS(1337-1337)Metadata(161-166)StepLease(1169-1176)pkg/api/constant.go (1)
DefaultIPPoolLeaseEnv(66-66)
🔇 Additional comments (18)
pkg/steps/release/promote.go (2)
108-112: LGTM! Sensible default version update.The fallback from
4.14to4.20is reasonable given the OCP5 enablement context. The new helper properly abstracts the stream iteration logic.
225-233: Clean implementation of multi-version stream fallback.The descending iteration from 9-stable to 4-stable ensures the latest available stable version is found. Errors from
prerelease.StableLatestMajorMinorare silently swallowed during iteration—this is acceptable for fallback logic, but could make debugging harder if all streams fail.pkg/release/prerelease/client.go (2)
51-58: LGTM!The refactored
stableLatestproperly accepts the stream parameter, enabling the caller to specify which stream to query.
60-70: LGTM! Good typo fix.The signature change to accept
streamenables multi-version support. The typo correction from "sematic" to "semantic" in the error message is a nice cleanup.pkg/api/leases_test.go (1)
87-116: Good 5.x branch coverage for AWS IP pool leases.These cases strengthen validation for new branch formats.
cmd/cvp-trigger/main.go (1)
157-202: Version format validation is clear and future-proof.The X.Y parsing with major >= 4 prevents ambiguous inputs.
cmd/registry-replacer/main.go (1)
68-98: Explicit current-release major/minor inputs look good.The validation aligns with the new CLI contract.
pkg/api/version_test.go (3)
105-137: ParseVersion test matrix looks solid.
202-261: GetAllPreviousVersions coverage is comprehensive.
263-315: GetAllPreviousVersionsSimple cases are clear and sufficient.cmd/branchingconfigmanagers/tide-config-manager/main_test.go (2)
613-620: Expanded 4.0 branch validation coverage looks good.
677-701: Added 5.x branch cases improve coverage.cmd/branchingconfigmanagers/tide-config-manager/main.go (1)
487-548: Generalized versioned-branch detection is clear and centralized.pkg/api/zz_generated.deepcopy.go (1)
1311-1324: ParsedVersion deepcopy addition looks correct.pkg/api/ocplifecycle/ocplifecycle.go (1)
269-276: UpdatedProvidesSignalForVersionsignature aligns with api types.pkg/api/version.go (2)
26-43:ParseVersionis clear and predictable.
Strict X.Y parsing with descriptive errors is solid.
10-15: Confirm behavior for initial minor releases (e.g., 4.1 → 4.0).
These helpers returnmajor.minor-1whenminor>0; if 4.0 is not a valid release, consider an explicit override or error to prevent consumers targeting a nonexistent stream.Also applies to: 45-168
pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.go (1)
101-128: Verify that Sippy release strings match X.Y format before parsing.
api.ParseVersionstrictly requires X.Y format (rejects X.Y.Z, single numbers, and non-numeric values). The code silently skips any release that fails parsing. If the Sippy API returns standard OpenShift releases with patch versions (e.g., "4.10.5" instead of "4.10"), those releases will be filtered out, potentially producing an incomplete or incorrect list of valid versions and wrong current/previous values. Either confirm Sippy returns only X.Y releases or normalize the format before parsing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| name: "4.1 computes previous as 4.0 (natural progression)", | ||
| version: "4.1", | ||
| availableVersions: availableVersions, | ||
| expected: "4.0", | ||
| }, |
There was a problem hiding this comment.
Update 4.1 expectations to skip the non-existent 4.0 release.
The version-transition rules treat 4.1 as the first 4.x release, so tests should not expect 4.0 as the previous version. Align these cases with the override (e.g., last 3.x).
🛠️ Proposed update
- {
- name: "4.1 computes previous as 4.0 (natural progression)",
- version: "4.1",
- availableVersions: availableVersions,
- expected: "4.0",
- },
+ {
+ name: "4.1 uses override to last 3.x (4.0 never existed)",
+ version: "4.1",
+ availableVersions: availableVersions,
+ expected: "3.11",
+ },- {
- name: "4.1 computes previous as 4.0 (natural progression)",
- version: "4.1",
- expected: "4.0",
- },
+ {
+ name: "4.1 uses override to last 3.x (4.0 never existed)",
+ version: "4.1",
+ expected: "3.11",
+ },Also applies to: 167-170
🤖 Prompt for AI Agents
In `@pkg/api/version_test.go` around lines 43 - 47, Update the test case named
"4.1 computes previous as 4.0 (natural progression)" in pkg/api/version_test.go
to expect the last 3.x release instead of "4.0" (the version-transition rules
treat 4.1 as the first 4.x so previous should fall back to the last 3.x from
availableVersions); replace the expected value that currently equals "4.0" with
the appropriate last 3.x entry from the availableVersions slice (and make the
analogous change for the similar test case around the other occurrence mentioned
at lines 167-170) so the assertions align with the override behavior.
There was a problem hiding this comment.
this is irrelevant due to age of 4.1
There was a problem hiding this comment.
@jmguzik Understood — given the age of 4.1, handling its transition accurately isn't a practical concern. Thanks for clarifying!
| // isValidVersion checks if a string represents a valid version like "4.15", "5.0", etc. | ||
| func isValidVersion(version string) bool { |
There was a problem hiding this comment.
Could we maybe add this do api.ocplifecycle? And then it could be used elsewhere. It seems you are doing a very similar check in all components.
There was a problem hiding this comment.
Or maybe into the version pkg you created
There was a problem hiding this comment.
yeah, I am trying to get rid of them in the future versions of this PR
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)
cmd/branchingconfigmanagers/frequency-reducer/main.go (1)
94-126: Cross‑major branches are misclassified as “older than past‑past”The
Less(Major, Minor-2)check at Line 99/Line 129 runs before the newpastPastVersion/pastVersionequality checks. For a current 5.0 release, all 4.x branches are< 5.-2, so Line 108–117 and Line 140–151 never execute. That makes 4.22/4.21 take the “older than past‑past” path and get monthly scheduling.Consider reordering the checks to evaluate the specific past versions first:
Suggested fix (reorder conditions)
- if testVersion.Less(ocplifecycle.MajorMinor{Major: version.Major, Minor: version.Minor - 2}) { - correctCron, err := isExecutedAtMostXTimesAMonth(*test.Cron, 1) - if err != nil { - logrus.Warningf("Can't parse cron string %s", *test.Cron) - continue - } - if !correctCron { - *test.Cron = generateMonthlyCron() - } - } else if pastPastVersion != "" && testVersion.GetVersion() == pastPastVersion { + if pastPastVersion != "" && testVersion.GetVersion() == pastPastVersion { correctCron, err := isExecutedAtMostXTimesAMonth(*test.Cron, 2) if err != nil { logrus.Warningf("Can't parse cron string %s", *test.Cron) continue } if !correctCron { *test.Cron = generateBiWeeklyCron() } - } else if pastVersion != "" && testVersion.GetVersion() == pastVersion { + } else if pastVersion != "" && testVersion.GetVersion() == pastVersion { correctCron, err := isExecutedAtMostXTimesAMonth(*test.Cron, 4) if err != nil { logrus.Warningf("Can't parse cron string %s", *test.Cron) continue } if !correctCron { *test.Cron = generateWeeklyWeekendCron() } + } else if testVersion.Less(ocplifecycle.MajorMinor{Major: version.Major, Minor: version.Minor - 2}) { + correctCron, err := isExecutedAtMostXTimesAMonth(*test.Cron, 1) + if err != nil { + logrus.Warningf("Can't parse cron string %s", *test.Cron) + continue + } + if !correctCron { + *test.Cron = generateMonthlyCron() + } }- if testVersion.Less(ocplifecycle.MajorMinor{Major: version.Major, Minor: version.Minor - 2}) { - duration, err := time.ParseDuration(*test.Interval) - if err != nil { - logrus.Warningf("Can't parse interval string %s", *test.Cron) - continue - } - if duration < time.Hour*24*28 { - cronExpr := generateWeeklyWeekendCron() - test.Cron = &cronExpr - test.Interval = nil - } - } else if pastPastVersion != "" && testVersion.GetVersion() == pastPastVersion { + if pastPastVersion != "" && testVersion.GetVersion() == pastPastVersion { duration, err := time.ParseDuration(*test.Interval) if err != nil { logrus.Warningf("Can't parse interval string %s", *test.Cron) continue } if duration < time.Hour*24*14 { cronExpr := generateBiWeeklyCron() test.Cron = &cronExpr test.Interval = nil } - } else if pastVersion != "" && testVersion.GetVersion() == pastVersion { + } else if pastVersion != "" && testVersion.GetVersion() == pastVersion { duration, err := time.ParseDuration(*test.Interval) if err != nil { logrus.Warningf("Can't parse interval string %s", *test.Cron) continue } if duration < time.Hour*24*7 { cronExpr := generateWeeklyWeekendCron() test.Cron = &cronExpr test.Interval = nil } + } else if testVersion.Less(ocplifecycle.MajorMinor{Major: version.Major, Minor: version.Minor - 2}) { + duration, err := time.ParseDuration(*test.Interval) + if err != nil { + logrus.Warningf("Can't parse interval string %s", *test.Cron) + continue + } + if duration < time.Hour*24*28 { + cronExpr := generateWeeklyWeekendCron() + test.Cron = &cronExpr + test.Interval = nil + } }Also applies to: 129-162
🤖 Fix all issues with AI agents
In `@pkg/api/version.go`:
- Around line 26-177: ParseVersion currently accepts negative components and
other funcs allow/produce the invalid 4.0 transition; fix by rejecting negative
major/minor in ParseVersion (return error if major<0 || minor<0) and then update
version-check and transition logic to special-case the 4.0/4.1 rule: in
IsValidOCPVersion require parsed.Major>=4 and reject the specific 4.0 case
(return false for 4.0), and in GetPreviousVersion, GetAllPreviousVersions,
GetPreviousVersionSimple, and GetAllPreviousVersionsSimple after
parsing/override checks, return an error when the computed or requested previous
version would be 4.0 or when current == "4.1" (unless an explicit override
exists in VersionTransitionOverrides), ensuring you consult
VersionTransitionOverrides first before applying the 4.1/4.0 block so explicit
overrides still work.
| func ParseVersion(version string) (ParsedVersion, error) { | ||
| parts := strings.Split(version, ".") | ||
| if len(parts) != 2 { | ||
| return ParsedVersion{}, fmt.Errorf("invalid version format: %s (expected X.Y)", version) | ||
| } | ||
|
|
||
| major, err := strconv.Atoi(parts[0]) | ||
| if err != nil { | ||
| return ParsedVersion{}, fmt.Errorf("invalid major version in %s: %w", version, err) | ||
| } | ||
|
|
||
| minor, err := strconv.Atoi(parts[1]) | ||
| if err != nil { | ||
| return ParsedVersion{}, fmt.Errorf("invalid minor version in %s: %w", version, err) | ||
| } | ||
|
|
||
| return ParsedVersion{Major: major, Minor: minor}, nil | ||
| } | ||
|
|
||
| // IsValidOCPVersion validates that the version is in format X.Y where X >= 4. | ||
| func IsValidOCPVersion(version string) bool { | ||
| parsed, err := ParseVersion(version) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return parsed.Major >= 4 | ||
| } | ||
|
|
||
| // GetPreviousVersion returns the primary previous version. For X.0 without override, | ||
| // it finds the highest (X-1).* from availableVersions. | ||
| func GetPreviousVersion(current string, availableVersions []string) (string, error) { | ||
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | ||
| return overrides[0], nil | ||
| } | ||
|
|
||
| parsed, err := ParseVersion(current) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if parsed.Minor > 0 { | ||
| return fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1), nil | ||
| } | ||
|
|
||
| if parsed.Major <= 0 { | ||
| return "", fmt.Errorf("cannot determine previous version for %s: no previous major version exists", current) | ||
| } | ||
|
|
||
| previousMajor := parsed.Major - 1 | ||
| var candidates []ParsedVersion | ||
|
|
||
| for _, v := range availableVersions { | ||
| pv, err := ParseVersion(v) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if pv.Major == previousMajor { | ||
| candidates = append(candidates, pv) | ||
| } | ||
| } | ||
|
|
||
| if len(candidates) == 0 { | ||
| return "", fmt.Errorf("cannot determine previous version for %s: no %d.x versions found in available versions", current, previousMajor) | ||
| } | ||
|
|
||
| sort.Slice(candidates, func(i, j int) bool { | ||
| return candidates[i].Minor > candidates[j].Minor | ||
| }) | ||
|
|
||
| return candidates[0].String(), nil | ||
| } | ||
|
|
||
| // GetAllPreviousVersions returns all valid previous versions from VersionTransitionOverrides, | ||
| // or natural progression if not in map. | ||
| func GetAllPreviousVersions(current string, availableVersions []string) ([]string, error) { | ||
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | ||
| return overrides, nil | ||
| } | ||
|
|
||
| parsed, err := ParseVersion(current) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if parsed.Minor > 0 { | ||
| return []string{fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1)}, nil | ||
| } | ||
|
|
||
| if parsed.Major <= 0 { | ||
| return nil, fmt.Errorf("cannot determine previous versions for %s: no previous major version exists", current) | ||
| } | ||
|
|
||
| previousMajor := parsed.Major - 1 | ||
| var candidates []ParsedVersion | ||
|
|
||
| for _, v := range availableVersions { | ||
| pv, err := ParseVersion(v) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if pv.Major == previousMajor { | ||
| candidates = append(candidates, pv) | ||
| } | ||
| } | ||
|
|
||
| if len(candidates) == 0 { | ||
| return nil, fmt.Errorf("cannot determine previous versions for %s: no %d.x versions found", current, previousMajor) | ||
| } | ||
|
|
||
| sort.Slice(candidates, func(i, j int) bool { | ||
| return candidates[i].Minor > candidates[j].Minor | ||
| }) | ||
|
|
||
| return []string{candidates[0].String()}, nil | ||
| } | ||
|
|
||
| // GetPreviousVersionSimple returns the primary previous version without availableVersions list. | ||
| // For X.0 without override, returns error. | ||
| func GetPreviousVersionSimple(current string) (string, error) { | ||
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | ||
| return overrides[0], nil | ||
| } | ||
|
|
||
| parsed, err := ParseVersion(current) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if parsed.Minor > 0 { | ||
| return fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1), nil | ||
| } | ||
|
|
||
| return "", fmt.Errorf("cannot determine previous version for %s: no override defined", current) | ||
| } | ||
|
|
||
| // GetAllPreviousVersionsSimple returns all valid previous versions without availableVersions list. | ||
| func GetAllPreviousVersionsSimple(current string) ([]string, error) { | ||
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | ||
| return overrides, nil | ||
| } | ||
|
|
||
| parsed, err := ParseVersion(current) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if parsed.Minor > 0 { | ||
| return []string{fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1)}, nil | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("cannot determine previous versions for %s: no override defined", current) | ||
| } |
There was a problem hiding this comment.
Prevent invalid 4.0/4.1 transitions and negative version components.
IsValidOCPVersion accepts 4.0, and GetPreviousVersion* returns 4.0 for 4.1, which conflicts with the “4.0 never existed” requirement and can produce invalid branches/inputs. Also, negative minors/majors currently parse as valid. Consider rejecting negative components and special-casing 4.1 to error out (or override explicitly).
Proposed fix
func ParseVersion(version string) (ParsedVersion, error) {
parts := strings.Split(version, ".")
if len(parts) != 2 {
return ParsedVersion{}, fmt.Errorf("invalid version format: %s (expected X.Y)", version)
}
major, err := strconv.Atoi(parts[0])
if err != nil {
return ParsedVersion{}, fmt.Errorf("invalid major version in %s: %w", version, err)
}
minor, err := strconv.Atoi(parts[1])
if err != nil {
return ParsedVersion{}, fmt.Errorf("invalid minor version in %s: %w", version, err)
}
+ if major < 0 || minor < 0 {
+ return ParsedVersion{}, fmt.Errorf("invalid version format: %s (negative numbers not allowed)", version)
+ }
return ParsedVersion{Major: major, Minor: minor}, nil
}
// IsValidOCPVersion validates that the version is in format X.Y where X >= 4.
func IsValidOCPVersion(version string) bool {
parsed, err := ParseVersion(version)
if err != nil {
return false
}
- return parsed.Major >= 4
+ if parsed.Major < 4 {
+ return false
+ }
+ // 4.0 never existed
+ if parsed.Major == 4 && parsed.Minor == 0 {
+ return false
+ }
+ return true
}
func GetPreviousVersion(current string, availableVersions []string) (string, error) {
if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 {
return overrides[0], nil
}
parsed, err := ParseVersion(current)
if err != nil {
return "", err
}
+ if parsed.Major == 4 && parsed.Minor == 1 {
+ return "", fmt.Errorf("cannot determine previous version for %s: 4.0 never existed", current)
+ }
if parsed.Minor > 0 {
return fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1), nil
}
@@
func GetAllPreviousVersions(current string, availableVersions []string) ([]string, error) {
if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 {
return overrides, nil
}
parsed, err := ParseVersion(current)
if err != nil {
return nil, err
}
+ if parsed.Major == 4 && parsed.Minor == 1 {
+ return nil, fmt.Errorf("cannot determine previous versions for %s: 4.0 never existed", current)
+ }
if parsed.Minor > 0 {
return []string{fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1)}, nil
}
@@
func GetPreviousVersionSimple(current string) (string, error) {
if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 {
return overrides[0], nil
}
parsed, err := ParseVersion(current)
if err != nil {
return "", err
}
+ if parsed.Major == 4 && parsed.Minor == 1 {
+ return "", fmt.Errorf("cannot determine previous version for %s: 4.0 never existed", current)
+ }
if parsed.Minor > 0 {
return fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1), nil
}
@@
func GetAllPreviousVersionsSimple(current string) ([]string, error) {
if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 {
return overrides, nil
}
parsed, err := ParseVersion(current)
if err != nil {
return nil, err
}
+ if parsed.Major == 4 && parsed.Minor == 1 {
+ return nil, fmt.Errorf("cannot determine previous versions for %s: 4.0 never existed", current)
+ }
if parsed.Minor > 0 {
return []string{fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1)}, nil
}📝 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.
| func ParseVersion(version string) (ParsedVersion, error) { | |
| parts := strings.Split(version, ".") | |
| if len(parts) != 2 { | |
| return ParsedVersion{}, fmt.Errorf("invalid version format: %s (expected X.Y)", version) | |
| } | |
| major, err := strconv.Atoi(parts[0]) | |
| if err != nil { | |
| return ParsedVersion{}, fmt.Errorf("invalid major version in %s: %w", version, err) | |
| } | |
| minor, err := strconv.Atoi(parts[1]) | |
| if err != nil { | |
| return ParsedVersion{}, fmt.Errorf("invalid minor version in %s: %w", version, err) | |
| } | |
| return ParsedVersion{Major: major, Minor: minor}, nil | |
| } | |
| // IsValidOCPVersion validates that the version is in format X.Y where X >= 4. | |
| func IsValidOCPVersion(version string) bool { | |
| parsed, err := ParseVersion(version) | |
| if err != nil { | |
| return false | |
| } | |
| return parsed.Major >= 4 | |
| } | |
| // GetPreviousVersion returns the primary previous version. For X.0 without override, | |
| // it finds the highest (X-1).* from availableVersions. | |
| func GetPreviousVersion(current string, availableVersions []string) (string, error) { | |
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | |
| return overrides[0], nil | |
| } | |
| parsed, err := ParseVersion(current) | |
| if err != nil { | |
| return "", err | |
| } | |
| if parsed.Minor > 0 { | |
| return fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1), nil | |
| } | |
| if parsed.Major <= 0 { | |
| return "", fmt.Errorf("cannot determine previous version for %s: no previous major version exists", current) | |
| } | |
| previousMajor := parsed.Major - 1 | |
| var candidates []ParsedVersion | |
| for _, v := range availableVersions { | |
| pv, err := ParseVersion(v) | |
| if err != nil { | |
| continue | |
| } | |
| if pv.Major == previousMajor { | |
| candidates = append(candidates, pv) | |
| } | |
| } | |
| if len(candidates) == 0 { | |
| return "", fmt.Errorf("cannot determine previous version for %s: no %d.x versions found in available versions", current, previousMajor) | |
| } | |
| sort.Slice(candidates, func(i, j int) bool { | |
| return candidates[i].Minor > candidates[j].Minor | |
| }) | |
| return candidates[0].String(), nil | |
| } | |
| // GetAllPreviousVersions returns all valid previous versions from VersionTransitionOverrides, | |
| // or natural progression if not in map. | |
| func GetAllPreviousVersions(current string, availableVersions []string) ([]string, error) { | |
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | |
| return overrides, nil | |
| } | |
| parsed, err := ParseVersion(current) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if parsed.Minor > 0 { | |
| return []string{fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1)}, nil | |
| } | |
| if parsed.Major <= 0 { | |
| return nil, fmt.Errorf("cannot determine previous versions for %s: no previous major version exists", current) | |
| } | |
| previousMajor := parsed.Major - 1 | |
| var candidates []ParsedVersion | |
| for _, v := range availableVersions { | |
| pv, err := ParseVersion(v) | |
| if err != nil { | |
| continue | |
| } | |
| if pv.Major == previousMajor { | |
| candidates = append(candidates, pv) | |
| } | |
| } | |
| if len(candidates) == 0 { | |
| return nil, fmt.Errorf("cannot determine previous versions for %s: no %d.x versions found", current, previousMajor) | |
| } | |
| sort.Slice(candidates, func(i, j int) bool { | |
| return candidates[i].Minor > candidates[j].Minor | |
| }) | |
| return []string{candidates[0].String()}, nil | |
| } | |
| // GetPreviousVersionSimple returns the primary previous version without availableVersions list. | |
| // For X.0 without override, returns error. | |
| func GetPreviousVersionSimple(current string) (string, error) { | |
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | |
| return overrides[0], nil | |
| } | |
| parsed, err := ParseVersion(current) | |
| if err != nil { | |
| return "", err | |
| } | |
| if parsed.Minor > 0 { | |
| return fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1), nil | |
| } | |
| return "", fmt.Errorf("cannot determine previous version for %s: no override defined", current) | |
| } | |
| // GetAllPreviousVersionsSimple returns all valid previous versions without availableVersions list. | |
| func GetAllPreviousVersionsSimple(current string) ([]string, error) { | |
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | |
| return overrides, nil | |
| } | |
| parsed, err := ParseVersion(current) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if parsed.Minor > 0 { | |
| return []string{fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1)}, nil | |
| } | |
| return nil, fmt.Errorf("cannot determine previous versions for %s: no override defined", current) | |
| } | |
| func ParseVersion(version string) (ParsedVersion, error) { | |
| parts := strings.Split(version, ".") | |
| if len(parts) != 2 { | |
| return ParsedVersion{}, fmt.Errorf("invalid version format: %s (expected X.Y)", version) | |
| } | |
| major, err := strconv.Atoi(parts[0]) | |
| if err != nil { | |
| return ParsedVersion{}, fmt.Errorf("invalid major version in %s: %w", version, err) | |
| } | |
| minor, err := strconv.Atoi(parts[1]) | |
| if err != nil { | |
| return ParsedVersion{}, fmt.Errorf("invalid minor version in %s: %w", version, err) | |
| } | |
| if major < 0 || minor < 0 { | |
| return ParsedVersion{}, fmt.Errorf("invalid version format: %s (negative numbers not allowed)", version) | |
| } | |
| return ParsedVersion{Major: major, Minor: minor}, nil | |
| } | |
| // IsValidOCPVersion validates that the version is in format X.Y where X >= 4. | |
| func IsValidOCPVersion(version string) bool { | |
| parsed, err := ParseVersion(version) | |
| if err != nil { | |
| return false | |
| } | |
| if parsed.Major < 4 { | |
| return false | |
| } | |
| // 4.0 never existed | |
| if parsed.Major == 4 && parsed.Minor == 0 { | |
| return false | |
| } | |
| return true | |
| } | |
| // GetPreviousVersion returns the primary previous version. For X.0 without override, | |
| // it finds the highest (X-1).* from availableVersions. | |
| func GetPreviousVersion(current string, availableVersions []string) (string, error) { | |
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | |
| return overrides[0], nil | |
| } | |
| parsed, err := ParseVersion(current) | |
| if err != nil { | |
| return "", err | |
| } | |
| if parsed.Major == 4 && parsed.Minor == 1 { | |
| return "", fmt.Errorf("cannot determine previous version for %s: 4.0 never existed", current) | |
| } | |
| if parsed.Minor > 0 { | |
| return fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1), nil | |
| } | |
| if parsed.Major <= 0 { | |
| return "", fmt.Errorf("cannot determine previous version for %s: no previous major version exists", current) | |
| } | |
| previousMajor := parsed.Major - 1 | |
| var candidates []ParsedVersion | |
| for _, v := range availableVersions { | |
| pv, err := ParseVersion(v) | |
| if err != nil { | |
| continue | |
| } | |
| if pv.Major == previousMajor { | |
| candidates = append(candidates, pv) | |
| } | |
| } | |
| if len(candidates) == 0 { | |
| return "", fmt.Errorf("cannot determine previous version for %s: no %d.x versions found in available versions", current, previousMajor) | |
| } | |
| sort.Slice(candidates, func(i, j int) bool { | |
| return candidates[i].Minor > candidates[j].Minor | |
| }) | |
| return candidates[0].String(), nil | |
| } | |
| // GetAllPreviousVersions returns all valid previous versions from VersionTransitionOverrides, | |
| // or natural progression if not in map. | |
| func GetAllPreviousVersions(current string, availableVersions []string) ([]string, error) { | |
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | |
| return overrides, nil | |
| } | |
| parsed, err := ParseVersion(current) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if parsed.Major == 4 && parsed.Minor == 1 { | |
| return nil, fmt.Errorf("cannot determine previous versions for %s: 4.0 never existed", current) | |
| } | |
| if parsed.Minor > 0 { | |
| return []string{fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1)}, nil | |
| } | |
| if parsed.Major <= 0 { | |
| return nil, fmt.Errorf("cannot determine previous versions for %s: no previous major version exists", current) | |
| } | |
| previousMajor := parsed.Major - 1 | |
| var candidates []ParsedVersion | |
| for _, v := range availableVersions { | |
| pv, err := ParseVersion(v) | |
| if err != nil { | |
| continue | |
| } | |
| if pv.Major == previousMajor { | |
| candidates = append(candidates, pv) | |
| } | |
| } | |
| if len(candidates) == 0 { | |
| return nil, fmt.Errorf("cannot determine previous versions for %s: no %d.x versions found", current, previousMajor) | |
| } | |
| sort.Slice(candidates, func(i, j int) bool { | |
| return candidates[i].Minor > candidates[j].Minor | |
| }) | |
| return []string{candidates[0].String()}, nil | |
| } | |
| // GetPreviousVersionSimple returns the primary previous version without availableVersions list. | |
| // For X.0 without override, returns error. | |
| func GetPreviousVersionSimple(current string) (string, error) { | |
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | |
| return overrides[0], nil | |
| } | |
| parsed, err := ParseVersion(current) | |
| if err != nil { | |
| return "", err | |
| } | |
| if parsed.Major == 4 && parsed.Minor == 1 { | |
| return "", fmt.Errorf("cannot determine previous version for %s: 4.0 never existed", current) | |
| } | |
| if parsed.Minor > 0 { | |
| return fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1), nil | |
| } | |
| return "", fmt.Errorf("cannot determine previous version for %s: no override defined", current) | |
| } | |
| // GetAllPreviousVersionsSimple returns all valid previous versions without availableVersions list. | |
| func GetAllPreviousVersionsSimple(current string) ([]string, error) { | |
| if overrides, ok := VersionTransitionOverrides[current]; ok && len(overrides) > 0 { | |
| return overrides, nil | |
| } | |
| parsed, err := ParseVersion(current) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if parsed.Major == 4 && parsed.Minor == 1 { | |
| return nil, fmt.Errorf("cannot determine previous versions for %s: 4.0 never existed", current) | |
| } | |
| if parsed.Minor > 0 { | |
| return []string{fmt.Sprintf("%d.%d", parsed.Major, parsed.Minor-1)}, nil | |
| } | |
| return nil, fmt.Errorf("cannot determine previous versions for %s: no override defined", current) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/api/version.go` around lines 26 - 177, ParseVersion currently accepts
negative components and other funcs allow/produce the invalid 4.0 transition;
fix by rejecting negative major/minor in ParseVersion (return error if major<0
|| minor<0) and then update version-check and transition logic to special-case
the 4.0/4.1 rule: in IsValidOCPVersion require parsed.Major>=4 and reject the
specific 4.0 case (return false for 4.0), and in GetPreviousVersion,
GetAllPreviousVersions, GetPreviousVersionSimple, and
GetAllPreviousVersionsSimple after parsing/override checks, return an error when
the computed or requested previous version would be 4.0 or when current == "4.1"
(unless an explicit override exists in VersionTransitionOverrides), ensuring you
consult VersionTransitionOverrides first before applying the 4.1/4.0 block so
explicit overrides still work.
There was a problem hiding this comment.
not relevant as 4.1 is realy old release
There was a problem hiding this comment.
@jmguzik Understood – given that 4.1 is indeed very old and the focus of this PR is on enabling support for OCP 5.x and beyond, it makes sense to prioritize practical concerns over edge cases that won't occur in practice. I appreciate the context!
|
Scheduling required tests: Scheduling tests matching the |
Signed-off-by: Jakub Guzik <[email protected]>
| return "", fmt.Errorf("cannot determine previous version for %s: no override defined", current) | ||
| } | ||
|
|
||
| // GetAllPreviousVersionsSimple returns all valid previous versions without availableVersions list. |
There was a problem hiding this comment.
Curious about the comment that it returns 'all valid previous versions' when it only returns the prior one, for clarity it should indicate it will only return the previous version. Also isn't this just a duplicate of GetPreviousVersionSimple, could it not just look through to that (or do you prefer it not)
| var toReleaseCandidates []string | ||
|
|
||
| if len(job.FromRelease.StringVal) > 0 { | ||
| candidates, err := api.GetAllPreviousVersionsSimple(job.FromRelease.StringVal) |
There was a problem hiding this comment.
Instead of getting candidates here (which will only be a single version for this api) could it call GetPreviousVersionSimple and get the single version string, then you don't need the nested for loops below like
for _, targetFromRelease := range fromReleaseCandidates {
for _, targetToRelease := range toReleaseCandidates {
| } | ||
|
|
||
| normalizedJobName := normalizeJobName(job.JobName, job.FromRelease.StringVal, job.Release) | ||
|
|
There was a problem hiding this comment.
If we can just get targetFromRelease, targetToRelease above via GetPreviousVersionSimple api then I don't think we need the changes below.
|
@jmguzik: The following tests failed, say
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. |
|
|
||
| // GetAllPreviousVersions returns all valid previous versions from VersionTransitionOverrides, | ||
| // or natural progression if not in map. | ||
| func GetAllPreviousVersions(current string, availableVersions []string) ([]string, error) { |
There was a problem hiding this comment.
It doesn't look like we need this yet:
$ git grep GetAllPreviousVersions[^S] origin/pr/4898 | grep -v _test
origin/pr/4898:pkg/api/version.go:// GetAllPreviousVersions returns all valid previous versions from VersionTransitionOverrides,
origin/pr/4898:pkg/api/version.go:func GetAllPreviousVersions(current string, availableVersions []string) ([]string, error) {And it's not clear to me if we did end up with some kind of 4.22 -> {5.0, 4.23} -> 5.1 diamond what we'd do with the results of this function. Currently GetAllPreviousVersionsSimple only seems to be used in getNormalizedFallBackJobName, and I'd expect 4.22 -> 5.0 and 5.0 -> 5.1 jobs to exist for use as fallbacks, without having to require access to a job that touched 4.23.
So maybe we can drop this function now to avoid dead code, and bring it back if it turns out I'm wrong and we do need it in the future? "Oh, we do need it, let's add it!" seems like something we'd notice, but "we didn't need it after all, let's delete that code" seems like something we might forget to do.
6 participants
This change prepares ci-tools for OpenShift 5.x support while maintaining backward compatibility with 4.x. It also generalizes version handling to support future major versions (6.x through 9.x).
Key changes:
Version Transition Logic (pkg/api/version.go):
Updated Components:
CLI Tools Updated:
The repository is now ready for OpenShift versions 4.x through 9.x.