fix: buggy nextY logic for maxOCPVersion
#3416
Merged
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
3 times, most recently
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
previously 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.YThere was a problem hiding this comment.
Ah good catch. Updated the PR: got the exact format you mentioned, see this
anik120
force-pushed
the
ocpMaxVersion-statement-clarification
branch
from
17f6109 to
a92cbec
Compare
|
New changes are detected. LGTM label has been removed. |
joelanford
reviewed
Oct 25, 2024
| @@ -105,8 +110,7 @@ func (s skew) String() string { | |||
| if s.err != nil { | |||
| return fmt.Sprintf("%s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err) | |||
There was a problem hiding this comment.
Suggested change
| return fmt.Sprintf("%s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err) | |
| return fmt.Sprintf("- %s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err) |
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
a92cbec to
cb9a1d3
Compare
joelanford
approved these changes
Oct 25, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Merged
via the queue into
operator-framework:master
with commit Oct 29, 2024
27ced56
12 checks passed
jianzhangbjz
reviewed
Nov 7, 2024
| // it is safe to ignore the error here, with the assumption | ||
| // that we build each skew object only after verifying that the | ||
| // version string is parseable safely. | ||
| maxOCPVersion, _ := semver.ParseTolerant(s[0].maxOpenShiftVersion) |
There was a problem hiding this comment.
Hi, will here be panic if the s is an empty slice?
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