Skip to content

Bug 2024433: CustomNoUpgrade should not fire no-upgrade failures #26615

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Dec 4, 2021
Merged

Bug 2024433: CustomNoUpgrade should not fire no-upgrade failures #26615

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions test/extended/operators/operators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,5 @@ func isKubeAPIUpgradableNoUpgradeCondition(cond configv1.ClusterOperatorStatusCo
return (cond.Reason == "FeatureGates_RestrictedFeatureGates_TechPreviewNoUpgrade" ||
cond.Reason == "FeatureGates_RestrictedFeatureGates_CustomNoUpgrade") &&
cond.Status == "False" &&
cond.Type == "Upgradeable" &&
cond.Message == "FeatureGatesUpgradeable: \"TechPreviewNoUpgrade\" and \"CustomNoUpgrades\" do not allow updates"
Copy link
Copy Markdown
Contributor

@deads2k deads2k Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the point of the string selection is to keep us from allowing techpreview clusters to go upgradeable=false for other reasons. Looks like the string is incorrect?

Copy link
Copy Markdown
Member

@wking wking Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the point of this conditional is to fail-fast-ish in cases where we are Upgradeable=False for unexpected reasons (it isn't limited to TechPreview). #26449, of which this is a robotic pick, says "you know what, who cares about the Message? (status, type, reason) is enough of a tuple for robots to be confident that this is an allowed reason". If you prefer to also watch over the operator's shoulder and enforce a specific message, I'm... grudgingly ok with that, but can you litigate it in master and then continue with the robotic backports to 4.9?

Copy link
Copy Markdown
Contributor

@bparees bparees Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the point of the string selection is to keep us from allowing techpreview clusters to go upgradeable=false for other reasons

doesn't checking the Reason ensure this, in a more robust(less fragile/subject to change) way?

Copy link
Copy Markdown
Contributor

@deads2k deads2k Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likely not more robust, but robust enough.

cond.Type == "Upgradeable"
}