fix: buggy nextY logic for maxOCPVersion
#3416
Open
Conversation
This file contains 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
openshift-ci
bot
added
the
do-not-merge/work-in-progress
anik120
force-pushed
the
ocpMaxVersion-statement-clarification
branch
from
cddccd8
to
99f83e1
Compare
anik120
changed the title
nextY logic for maxOCPVersion
anik120
removed
the
do-not-merge/work-in-progress
anik120
force-pushed
the
ocpMaxVersion-statement-clarification
branch
2 times, most recently
from
79c1b19
to
dca0e03
Compare
The handling logic of versions that are pre-releases by the
nextY() func (that determines the next Y release) was erroneous.
Eg: nextY("4.16.0") returns "4.17" correctly, but nextY("4.16.0-rc1")
returns "4.16" (the correct value is still "4.17").
This PR fixes the nextY function.
Also has improvement for the "not-upgradeable to next OCP" version message.
anik120
force-pushed
the
ocpMaxVersion-statement-clarification
branch
from
dca0e03
to
17f6109
Compare
anik120
commented
Oct 22, 2024
Comment on lines
+108
to
+113
| // is it safe to ignore the error here, with the assumption | ||
| // that we build a skew object only after verifying that the | ||
| // version string is parseable safely. | ||
| maxOCPVersion, _ := semver.ParseTolerant(s.maxOpenShiftVersion) | ||
| nextY := nextY(maxOCPVersion).String() | ||
| return fmt.Sprintf("ClusterServiceVersions blocking upgrade to %s or higher. The maximum supported OCP version for %s/%s is %s", nextY, s.namespace, s.name, s.maxOpenShiftVersion) |
There was a problem hiding this comment.
For ref, this is where the skew objects are created, where s.maxOpenshiftVersion is set AFTER the value of the field from the CSV is parsed here, and maxOpenshiftVersion() checks that the value is parseable and returns the error if it's not
|
/lgtm |
grokspawn
approved these changes
Oct 22, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
joelanford
reviewed
Oct 23, 2024
| // version string is parseable safely. | ||
| maxOCPVersion, _ := semver.ParseTolerant(s.maxOpenShiftVersion) | ||
| nextY := nextY(maxOCPVersion).String() | ||
| return fmt.Sprintf("ClusterServiceVersions blocking upgrade to %s or higher. The maximum supported OCP version for %s/%s is %s", nextY, s.namespace, s.name, s.maxOpenShiftVersion) |
There was a problem hiding this comment.
Am I reading things correctly that this message is printed for each CSV that is blocking the upgrade. If so, it would be duplicative to repeatedly state in our Upgradeable=False message that "ClusterServiceVersions are blocking minor version upgrades to X.Y+1 or higher"
WDYT about the final message looking something like this?
conditions:
- type: Upgradeable
status: False
reason: IncompatibleOperators
message: |
ClusterServiceVersions are blocking minor version upgrades to X.Y+1 or higher.
- openshift-operators/foo is supported through X.Y
- openshift-operators/bar is supported through X.Y
- openshift-operators/baz is supported through X.Y
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The handling logic of versions that are pre-releases by the
nextY()func (that determines the next Y release) was erroneous.Eg:
nextY("4.16.0")returns "4.17" correctly, butnextY("4.16.0-rc1")returns "4.16" (the correct value is still "4.17").
This PR fixes the
nextYfunction.Also has improvement for the "not-upgradeable to next OCP" version message.
Description of the change:
Motivation for the change:
Architectural changes:
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue