-
Notifications
You must be signed in to change notification settings - Fork 544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: buggy nextY
logic for maxOCPVersion
#3416
base: master
Are you sure you want to change the base?
fix: buggy nextY
logic for maxOCPVersion
#3416
Conversation
cddccd8
to
99f83e1
Compare
nextY
logic for maxOCPVersion
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.
dca0e03
to
17f6109
Compare
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
nextY
function.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