Bug 2056893: pkg/cli/admin/upgrade: Drop --to-image help warning #1078
Conversation
wking
force-pushed
the
soften-upgrade-to-image-warning
branch
from
b754c4b to
b6bb202
Compare
openshift-ci
bot
added
the
approved
wking
changed the title
openshift-ci
bot
added
bugzilla/severity-low
|
@wking: This pull request references Bugzilla bug 2056893, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
pkg/cli/admin/upgrade/upgrade.go
Outdated
| If the cluster is already being upgraded, or if the cluster is reporting a failure or | ||
| other error, you must pass --allow-upgrade-with-warnings to proceed (see note below on | ||
| the implications). Avoid upgrading when the cluster is reporting errors or when another | ||
| upgrade is in progress unless the upgrade cannot make progress. |
There was a problem hiding this comment.
Avoid upgrading when the cluster is reporting errors or when another upgrade is in progress unless the upgrade cannot make progress. is confusing and do not communicate the context properly. Here is my suggestion You should understand risks from the error/warnings before considering to use --allow-upgrade-with-warnings oe something like that. I am looking for a smaller sentence here.
There was a problem hiding this comment.
Addressed with b6bb202 -> 4e3339f:
If the cluster is already being upgraded, or if the cluster is reporting a failure or other error, the update will not be triggered. It is usually best to give these conditions time to resolve, or to actively work to resolve them. But if you decide to trigger the update regardless of these concerns, use --allow-upgrade-with-warnings.
We've had the warning at least since this repo was created in 3abf60a (setup oc repo, 2019-06-17). But since we grew --allow-explicit-upgrade in 0501d04 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift#109), --to-image has been just as restricted to recommended updates as --to. Removing the warning here avoids unnecessary alarm. We can save the warning for --allow-explicit-upgrade, whose help text I'm consolidating in this commit. The new structure is: * --to and --to-image together, since in the absence of --allow-explicit-upgrade those are equally safe. * The paragraph about unsupported updates or broken CVO <-> upstream update service communication. Before this paragraph had focused on --to-image, but I've adjusted it to focus on --allow-explicit-upgrade. * The --allow-upgrade-with-warnings paragraph. I've moved the sentence about retargeting updates over to this paragraph, since --allow-upgrade-with-warnings is the relevant flag for that issue. * The --force paragraph, because that's about a cluster-side check that occurs after the oc-side --allow-* checks, so having it last in the help text gives us the same order in help as we get for check application.
wking
force-pushed
the
soften-upgrade-to-image-warning
branch
from
b6bb202 to
4e3339f
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LalatenduMohanty, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
|
@wking: An error was encountered updating to the MODIFIED state for bug 2056893 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
code 32000: Sub-component is mandatory for the component 'oc' in the product 'OpenShift Container Platform'.
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn 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. |
4 participants
We've had the warning at least since this repo was created in 3abf60a. But since we grew
--allow-explicit-upgradein 0501d04 (#109),--to-imagehas been just as restricted to recommended updates as--to. Removing the warning here avoids unnecessary alarm. We can save the warning for--allow-explicit-upgrade, whose help text I'm consolidating in thiscommit. The new structure is:
--toand--to-imagetogether, since in the absence of--allow-explicit-upgradethose are equally safe.The paragraph about unsupported updates or broken CVO <-> upstream update service communication. Before this paragraph had focused on
--to-image, but I've adjusted it to focus on--allow-explicit-upgrade.The
--allow-upgrade-with-warningsparagraph. I've moved the sentence about retargeting updates over to this paragraph, since--allow-upgrade-with-warningsis the relevant flag for that issue.The
--forceparagraph, because that's about a cluster-side check that occurs after the oc-side--allow-*checks, so having it last in the help text gives us the same order in help as we get for check application.