[poc] honor UpgradeableMinor condition #285
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
size/L
|
/hold this is just a poc |
openshift-ci-robot
added
the
do-not-merge/hold
deads2k
force-pushed
the
super-dirty-upgradeableminor
branch
from
60e5a15 to
2038a58
Compare
| type Precondition interface { | ||
| // Run executes the precondition checks ands returns an error when the precondition fails. | ||
| Run(context.Context) error | ||
| Run(ctx context.Context, desiredVersion string) error |
There was a problem hiding this comment.
If we're doing this can we just unify the loops for preconditions?
There was a problem hiding this comment.
(I don't care that much, but there's a lot of duplicate machinery)
There was a problem hiding this comment.
I could if that was desired. Most of this is copy/pasta. This was mostly about proving that the effort is fairly easy/straightforward, so we can argue about whether there is a need that can be easily met and not get distracted by the amount of work.
|
@deads2k: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
|
Alright, alternate pitch, what if Upgradeable actually meant "cannot upgrade y, but can upgrade z"? We set it for...
I don't know of another case where we set it offhand |
|
That would be helpful for removing items that are deprecated but still in use IIUC. I assume though that this would not help if I was removing something in 4.4 because the 4.3 CVO won’t have this bit? |
|
The original argument was to only do minor upgrade blocking (since that’s what actually matters to most end users) - the “no z upgrade for tech preview” was a hardline stronger than our requirements. I’d be willing to consider redefining upgradeable to mean “minor” given that and the history of why we did this in the first place. Needs a bit of F2F discussion though about possible downsides. |
|
closed in favor of #291 |
4 participants
Just an experiment to see how much violence is required to support an
UpgradeableMinorcondition. This is a very ugly example of how to do it, but it looks pretty safe to do.