OTA-932: pkg/cli/admin/update: Rename "upgrade" to "update" and similar #1472

wking · 2023-06-15T18:56:12Z

The outgoing upgrade phrasing still works, but now logs deprecation warnings. We may never remove the outgoing upgrade phrasing, because it seems unlikely to be worth the compatibility churn. But moving the canonical phrasing to update aligns with openshift/openshift-docs@a68b607325 (openshift/openshift-docs#40525). And logging the deprecation warnings makes the new canonical phrasing more obvious, which should help gradually transition existing consumers, because the more consistently folks use the canonical phrasing, the easier it is to find it with grep-searches when searching scripts and similar.

openshift-ci-robot · 2023-06-15T18:56:30Z

@wking: This pull request references OTA-932 which is a valid jira issue.

Details

In response to this:

The outgoing upgrade phrasing still works, but now logs deprecation warnings. We may never remove the outgoing upgrade phrasing, because it seems unlikely to be worth the compatibility churn. But moving the canonical phrasing to update aligns with openshift/openshift-docs@a68b607325 (openshift/openshift-docs#40525). And logging the deprecation warnings makes the new canonical phrasing more obvious, which should help gradually transition existing consumers, because the more consistently folks use the canonical phrasing, the easier it is to find it with grep-searches when searching scripts and similar.

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.

wking · 2023-06-15T21:54:46Z

Testing with a local build:

$ go build ./cmd/oc
$ ./oc adm update  # new status approach
Cluster version is 4.13.2
...
$ ./oc adm upgrade  # old status approach, confirming a warning is logged
oc adm update is deprecated.  Use 'oc adm update ...' instead
Cluster version is 4.13.2
...
$ ./oc adm update --allow-upgrade-with-warnings --to 4.99.99  # two command line flags also moved, and using them gets a warning logged, but isn't fatal (the non-existed release used for testing is what fails this call)
Flag --allow-upgrade-with-warnings has been deprecated, please use --allow-update-with-warnings instead
error: the update is not one of the possible targets: 4.13.3. specify --to-image to continue with the update.
$ ./oc adm upgrade --help | grep -1 upgrade  # the deprecated options are hidden
Aliases:
update, upgrade

So that looks good to me :)

wking · 2023-06-16T14:44:38Z

Rebased around #1341 to resolve diff-context-overlap with 2098a85 -> 7b95972.

cgwalters · 2023-06-16T17:05:29Z

What's the motivation behind logging deprecation warnings? I mean really:

we may never remove the outgoing upgrade phrasing, because it seems unlikely to be worth the compatibility churn.

Exactly. Many tools (e.g. dnf) supports both dnf upgrade and dnf update as aliases, we did the same change for rpm-ostree a while ago.

If we're never going to remove it, let's not nag users to change pointlessly.

cgwalters · 2023-06-16T18:38:22Z

Confusingly, apt update and apt upgrade are totally different things. It looks like zypper update exists, but not zypper upgrade.

LalatenduMohanty · 2023-06-16T19:40:38Z

pkg/cli/admin/update/update.go

 			kcmdutil.CheckErr(o.Complete(f, cmd, args))
 			kcmdutil.CheckErr(o.Run())
 		},
+		Aliases: []string{"upgrade"},


Deleted my previous comment about the warning message about upgraded because I saw that we already printing the message.

LalatenduMohanty · 2023-06-16T19:57:19Z

If we're never going to remove it, let's not nag users to change pointlessly.

The change from upgrade to update is not single sub-command change as we have used upgrade string in other places e.g. flags etc. So we wanted to push folks to use the updated names without breaking older scripts.

But I can live without the warning message as well, if you think strongly about it.

$ oc adm upgrade --help | grep -i upgrade
Check on upgrade status or upgrade the cluster to a newer version
 This command assists with cluster upgrades. If no arguments are passed the command will retrieve the current version info and display whether an upgrade is in progress or whether any errors might prevent an upgrade, as well as show the suggested updates available to the cluster. Information about compatible updates is periodically retrieved from the update server and cached on the cluster - these are updates that are known to be supported as upgrades from the current version.
 Passing --to=VERSION or --to-image=IMAGE will upgrade the cluster to one of the available updates or report an error if no such version exists. The cluster will then upgrade itself and report status that is available via "oc get clusterversion" and "oc describe clusterversion".
 Passing --to-multi-arch will upgrade the cluster from a single-architecture to a multi-architecture cluster at the current version.
 If there are no versions available, or a bug in the cluster version operator prevents updates from being retrieved, --to-image may be combined with the more powerful and dangerous --allow-explicit-upgrade. This instructs the cluster to upgrade to the contents of the specified release image, regardless of whether that upgrade is known to be recommended for the current version. While rolling back to a previous patch (z stream) version (4.1.2 -> 4.1.1) may be safe, upgrading more than one minor version ahead (4.1 -> 4.3) or downgrading one minor version (4.2 -> 4.1) is likely to cause data corruption or to completely break a cluster.
 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.
 The cluster may report that the upgrade should not be performed due to a content verification error or update precondition failures such as operators blocking upgrades. Do not upgrade to images that are not appropriately signed without understanding the risks of upgrading your cluster to untrusted code. If you must override this protection use the --force flag.
  oc adm upgrade
  oc adm upgrade --to-latest=true
    --allow-explicit-upgrade=false:
	Upgrade even if the upgrade target is not listed in the available versions list.
	Allows upgrade to a version when it is supported but not recommended for updates.
    --allow-upgrade-with-warnings=false:
	Upgrade even if an upgrade is in process or a cluster error is blocking the update.
	If an upgrade has been requested but not yet downloaded, cancel the update. This has no effect once the update has started.
	Forcefully upgrade the cluster even when upgrade release image validation fails and the cluster is reporting errors.
	Specify the version to upgrade to. The version must be on the list of available updates.
	Provide a release image to upgrade to.
	Upgrade current version to multi architecture.
  oc adm upgrade --to=VERSION [flags] [options]

LalatenduMohanty

/lgtm

wking · 2023-06-16T20:26:23Z

If we're never going to remove it, let's not nag users to change pointlessly.

I agree that the update is pointless as far as "does the invocation work" goes. But as I point out briefly in my initial post, there are ecosystem benefits to consistency. The more widespread the canonical phrasing is, the less mental overhead there is among folks sharing scripts, documenting behavior, and so on. The logged stderr messages will do a little bit to gradually shift existing call-sites over to the new canonical phrasing, and make it less likely that new callers chose the outdated format, and gradually the bulk of users will not even need to be aware that the outdated (but still functional) phrasing ever existed.

pkg/cli/admin/update/update.go

evakhoni · 2023-06-19T10:05:51Z

pkg/cli/admin/update/update.go

 		`),
+		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
+			if cmd.CalledAs() != "update" {
+				fmt.Fprintf(o.ErrOut, "%s is deprecated.  Use '%s ...' instead\n", cmd.CommandPath(), cmd.CommandPath())


❯ oc adm upgrade oc adm update is deprecated. Use 'oc adm update ...' instead

..., cmd.CommandPath(), cmd.CommandPath()) seems wrong. we're using the same string twice

I've added a Replace to replace update with upgrade in 003a50d -> 5f78beb, which also rebases around #1330:

$ ./oc adm upgrade oc adm upgrade is deprecated. Use 'oc adm update ...' instead Cluster version is 4.13.2 ....

evakhoni · 2023-06-19T10:10:02Z

/hold
seems like an issue to solve before merging

evakhoni · 2023-06-19T10:19:18Z

pkg/cli/admin/update/update.go

 			use --force, which is passed through to ClusterVersion's spec.desiredUpdate.force.
 		`),
+		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
+			if cmd.CalledAs() != "update" {


❯ oc adm update channel oc adm update channel is deprecated. Use 'oc adm update channel ...' instead info: Cluster channel is already clear (no change)

hmm.. we're not deprecating the channel subcommand right?

Thanks! Fixed in 003a50d -> 5f78beb:

$ ./oc adm upgrade channel oc adm upgrade channel is deprecated. Use 'oc adm update channel ...' instead error: You are requesting to clear the update channel. The current channel "candidate-4.13" is one of the available channels, you must pass --allow-explicit-channel to continue

retested - update channel still triggering a false positive!

❯ oc adm update channel oc adm upgrade channel is deprecated. Use 'oc adm update channel ...' instead info: Cluster channel is already clear (no change)

btw, there's a double space in the middle - intentional?

openshift-ci · 2023-06-20T16:00:11Z

New changes are detected. LGTM label has been removed.

The outgoing "upgrade" phrasing still works, but now logs deprecation warnings. We may never remove the outgoing "upgrade" phrasing, because it seems unlikely to be worth the compatibility churn. But moving the canonical phrasing to "update" aligns with openshift/openshift-docs@a68b607325 (Adding 'update' to the glossary, 2022-01-22, openshift/openshift-docs#40525). And logging the deprecation warnings makes the new canonical phrasing more obvious, which should help gradually transition existing consumers, because the more consistently folks use the canonical phrasing, the easier it is to find it with grep-searches when searching scripts and similar.

wking · 2023-06-20T22:13:54Z

build05 registry 504s.

/retest

ardaguclu · 2023-06-21T05:40:07Z

I dropped my comments on Slack but wanted to write also in here;

I'd highly suggest deprecating upgrade command instead directly removing it. There is a version compatibility skew(and this PR's current status breaks this, oc adm upgrade 4.14 won't work on 4.13).

Instead we can deprecate and wait at least 1 releases for users to update their scripts to use new update command.

ardaguclu

/approve

ardaguclu · 2023-06-26T07:35:35Z

pkg/cli/admin/release/info.go

 			Specify one or more images via pull spec to see details of each release image. You may also
 			pass a semantic version (4.11.2) as an argument, and if cluster version object has seen such a
-			version in the upgrades channel it will find the release info for that version.
+			version in the update channel it will find the release info for that version.


nit:

Suggested change

version in the update channel it will find the release info for that version.

version in the updates channel it will find the release info for that version.

ardaguclu · 2023-06-26T07:37:16Z

I'd prefer to see the e2e-metal-ipi-ovn-ipv6 is also green. But since it is not required job;
/hold
/retest

openshift-ci · 2023-06-26T07:39:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, LalatenduMohanty, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ardaguclu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-06-26T10:39:32Z

@wking: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-metal-ipi-ovn-ipv6	`5f78beb`	link	false	`/test e2e-metal-ipi-ovn-ipv6`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

sdodson · 2023-06-28T18:15:43Z

/hold
I know this is already held, but just noting here that there may be a desire to offer considerably different UX around oc adm update versus the deprecated oc adm upgrade such as forcing specification of intent to update to latest z-stream or minor version, etc.

Will leave a note on the card as well.

openshift-bot · 2023-09-27T01:00:32Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-merge-robot · 2023-09-27T01:00:47Z

PR needs rebase.

Details

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.

LalatenduMohanty · 2023-10-24T13:39:34Z

We are not planning to rename upgrade to update. Rather we would re-imagine a new user experience for update sub-command, hence closing this PR

LalatenduMohanty · 2023-10-24T13:39:40Z

/close

openshift-ci · 2023-10-24T13:46:56Z

@LalatenduMohanty: Closed this PR.

Details

In response to this:

/close

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.

wking changed the title ~~pkg/cli/admin/update: Rename "upgrade" to "update" and similar~~ OTA-932: pkg/cli/admin/update: Rename "upgrade" to "update" and similar Jun 15, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 15, 2023

openshift-ci bot requested review from mfojtik and soltysh June 15, 2023 18:56

wking force-pushed the oc-adm-upgrade-to-update branch 3 times, most recently from 3b0292c to d258461 Compare June 15, 2023 20:50

wking force-pushed the oc-adm-upgrade-to-update branch from d258461 to 2098a85 Compare June 15, 2023 21:56

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2023

wking force-pushed the oc-adm-upgrade-to-update branch from 2098a85 to 7b95972 Compare June 16, 2023 14:44

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2023

wking force-pushed the oc-adm-upgrade-to-update branch from 7b95972 to 003a50d Compare June 16, 2023 18:33

LalatenduMohanty reviewed Jun 16, 2023

View reviewed changes

LalatenduMohanty approved these changes Jun 16, 2023

View reviewed changes

openshift-ci bot assigned LalatenduMohanty Jun 16, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2023

LalatenduMohanty mentioned this pull request Jun 16, 2023

OTA-981: pkg/cli/admin/upgrade/status: Add a subcommand just for status #1470

Closed

evakhoni reviewed Jun 18, 2023

View reviewed changes

pkg/cli/admin/update/update.go Show resolved Hide resolved

evakhoni reviewed Jun 19, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 19, 2023

evakhoni reviewed Jun 19, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2023

wking force-pushed the oc-adm-upgrade-to-update branch from 003a50d to e4079c1 Compare June 20, 2023 16:00

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2023

wking force-pushed the oc-adm-upgrade-to-update branch from e4079c1 to 220d10e Compare June 20, 2023 16:02

wking force-pushed the oc-adm-upgrade-to-update branch from 220d10e to 5f78beb Compare June 20, 2023 16:38

ardaguclu reviewed Jun 26, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2023

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 27, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2023

openshift-ci bot closed this Oct 24, 2023

	version in the update channel it will find the release info for that version.
	version in the updates channel it will find the release info for that version.

OTA-932: pkg/cli/admin/update: Rename "upgrade" to "update" and similar #1472

OTA-932: pkg/cli/admin/update: Rename "upgrade" to "update" and similar #1472

Uh oh!

Conversation

wking commented Jun 15, 2023

Uh oh!

openshift-ci-robot commented Jun 15, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Jun 15, 2023

Uh oh!

wking commented Jun 16, 2023

Uh oh!

cgwalters commented Jun 16, 2023

Uh oh!

cgwalters commented Jun 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LalatenduMohanty commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LalatenduMohanty left a comment

Choose a reason for hiding this comment

Uh oh!

wking commented Jun 16, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evakhoni commented Jun 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 20, 2023

Uh oh!

wking commented Jun 20, 2023

Uh oh!

ardaguclu commented Jun 21, 2023

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardaguclu commented Jun 26, 2023

Uh oh!

openshift-ci bot commented Jun 26, 2023

Uh oh!

openshift-ci bot commented Jun 26, 2023

Uh oh!

sdodson commented Jun 28, 2023

Uh oh!

openshift-bot commented Sep 27, 2023

Uh oh!

openshift-merge-robot commented Sep 27, 2023

Uh oh!

LalatenduMohanty commented Oct 24, 2023

Uh oh!

LalatenduMohanty commented Oct 24, 2023

Uh oh!

openshift-ci bot commented Oct 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

openshift-ci-robot commented Jun 15, 2023 •

edited by openshift-ci bot

Loading

LalatenduMohanty commented Jun 16, 2023 •

edited

Loading

wking Jun 20, 2023 •

edited

Loading