clarify maxOpenShiftVersion upgrade error message #2326
Conversation
| @@ -110,7 +110,7 @@ func (s skew) String() string { | |||
| return fmt.Sprintf("%s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err) | |||
| } | |||
|
|
|||
| return fmt.Sprintf("%s/%s is incompatible with OpenShift versions greater than %s", s.namespace, s.name, s.maxOpenShiftVersion) | |||
| return fmt.Sprintf("%s/%s is incompatible with OpenShift minor versions greater than %s", s.namespace, s.name, s.maxOpenShiftVersion) | |||
There was a problem hiding this comment.
I believe skew.maxOpenShiftVersion is the result of directly stringifying the value of the property, which can specify patch versions. I would take care to format the message so that only major.minor is displayed.
There was a problem hiding this comment.
Could the test include a maxOpenShift version that includes a patch and could we ensure that the patch version is omitted from the message?
There was a problem hiding this comment.
Shouldn't we just throw a validation error on a patch version? It doesn't seem to me to make a lot of sense to allow users to even set it like that if we can't actually block those upgrades anyway.
There was a problem hiding this comment.
How about requiring the operators to declare maxOpenShiftVersion: 4.8.9999 if they support all future 4.8.* versions?
There was a problem hiding this comment.
I just think it's confusing and erroneous to do that. OLM functionally can't do anything special with the patch version here, so we should limit it to the minor version with validation.
There was a problem hiding this comment.
Modified it to drop the build version and complain on a non-zero patch/pre-release version. I've added a warning on the api validation operator-framework/api#146
|
@ankitathomas thanks for the quick turnaround on this, one suggestion on the test, looks good otherwise. |
ankitathomas
force-pushed
the
maxopenshiftversion-errormsg-fix
branch
2 times, most recently
from
35f113f to
51493ee
Compare
ankitathomas
force-pushed
the
maxopenshiftversion-errormsg-fix
branch
from
51493ee to
fc58b6a
Compare
openshift-ci
bot
added
the
needs-rebase
ankitathomas
force-pushed
the
maxopenshiftversion-errormsg-fix
branch
from
fc58b6a to
9fdf0df
Compare
openshift-ci
bot
removed
the
needs-rebase
Signed-off-by: Ankita Thomas <[email protected]>
ankitathomas
force-pushed
the
maxopenshiftversion-errormsg-fix
branch
from
9fdf0df to
3467201
Compare
| @@ -2,7 +2,6 @@ package openshift | |||
|
|
|||
| import ( | |||
| "fmt" | |||
|
|
|||
There was a problem hiding this comment.
nit: this newline should exist to delineate standard library imports.
openshift-ci
bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankitathomas, dinhxuanvu, njhale The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retitle Bug 1992677: clarify maxOpenShiftVersion upgrade error message |
openshift-ci
bot
changed the title
|
@ankitathomas: All pull requests linked via external trackers have merged: Bugzilla bug 1992677 has been moved to the MODIFIED state. In response to this:
Instructions 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/test-infra repository. |
|
/retitle clarify maxOpenShiftVersion upgrade error message |
openshift-ci
bot
changed the title
Signed-off-by: Ankita Thomas [email protected]