Skip to content

OSDOCS-16102 Adding oc adm upgrade recommend to updating with cli#98609

Merged
jeana-redhat merged 1 commit intoopenshift:mainfrom
cbippley:OSDOCS-15792
Oct 14, 2025
Merged

OSDOCS-16102 Adding oc adm upgrade recommend to updating with cli#98609
jeana-redhat merged 1 commit intoopenshift:mainfrom
cbippley:OSDOCS-15792

Conversation

@cbippley
Copy link
Contributor

@cbippley cbippley commented Sep 5, 2025

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 5, 2025
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 5, 2025
@cbippley cbippley force-pushed the OSDOCS-15792 branch 2 times, most recently from 53be072 to 006908e Compare September 5, 2025 17:31
@cbippley cbippley changed the title OSDOCS-15792 Adding oc adm upgrade recommend to updating with cli OSDOCS-16102 Adding oc adm upgrade recommend to updating with cli Sep 8, 2025
@cbippley cbippley force-pushed the OSDOCS-15792 branch 5 times, most recently from 635bd0e to 3feb8fe Compare September 9, 2025 19:46
Copy link
Contributor

@skopacz1 skopacz1 left a comment

Choose a reason for hiding this comment

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

Left a few comments and hopefully that will help you out. Also, I will quickly define the different conditionals and what they mean, in case it's helpful reference:

  • ifdef::condition[] - this means "if condition is defined wherever this module is included, show everything between here and the endif" (i.e. "include in condition assemblies but exclude everywhere else")
  • ifndef::condition[] - this means "if condition is not defined wherever this module is included, show everything between here and the endif" (i.e. "exclude from condition assemblies but show everywhere else")
  • endif::condition[] - this is like a closing tag, basically "everything past this line is no longer part of either the ifdef or ifndef for condition". You can also have this without a condition specified, like endif::[], and I think it just closes whatever was the most recent ifdef or endif, but it's generally good to specify what condition it should apply to

@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 10, 2025
Copy link
Contributor

@skopacz1 skopacz1 left a comment

Choose a reason for hiding this comment

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

This is the one thing that's sticking out to me right now

@cbippley cbippley force-pushed the OSDOCS-15792 branch 2 times, most recently from e634217 to 48f81bd Compare September 17, 2025 18:23
[NOTE]
====
Your cluster does not need to be a Technology Preview-enabled cluster in order for you to use the `oc adm upgrade recommend` command.
Your cluster does not need to be a Technology Preview-enabled cluster in order for you to use the `oc adm upgrade recommend` command.

Choose a reason for hiding this comment

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

Once we GA recommend, I don't think we need this notification, right? cc @wking

Copy link
Member

Choose a reason for hiding this comment

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

Right, this whole file is getting a [removed from assembly] comment up on line 3. I don't understand why we just flag files as removed from assemblies instead of just removing the file, but as far as this pull is concerned, this whitespace removal is just tidying up dead-code, and 🤷, that doesn't hurt or help anything

[source,terminal]
----
$ oc adm upgrade
$ oc adm upgrade status

Choose a reason for hiding this comment

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

cc @dis016 for the status sub-command.

@cbippley
Copy link
Contributor Author

cbippley commented Oct 7, 2025

/retest

@cbippley cbippley force-pushed the OSDOCS-15792 branch 2 times, most recently from 71957bd to 9ac7038 Compare October 7, 2025 18:19
@cbippley cbippley force-pushed the OSDOCS-15792 branch 2 times, most recently from 4dde665 to 06d8bb9 Compare October 9, 2025 19:09
Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 9, 2025
@openshift-ci
Copy link

openshift-ci bot commented Oct 9, 2025

New changes are detected. LGTM label has been removed.

@JianLi-RH
Copy link

For the recommend command (excluding precheck), LGTM

[source,terminal]
----
$ oc adm upgrade
$ oc adm upgrade status
Copy link

Choose a reason for hiding this comment

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

@cbippley can we have the example output section for during the upgrade? cc @DavidHurta @wking

@cbippley cbippley force-pushed the OSDOCS-15792 branch 3 times, most recently from 78c4e7c to ff8e122 Compare October 13, 2025 17:38
@dis016
Copy link

dis016 commented Oct 14, 2025

/lgtm for status sub-command

@cbippley
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Oct 14, 2025
@jeana-redhat jeana-redhat added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Oct 14, 2025
Copy link
Contributor

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

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

Some minor things to look at here. Most important is to fix that formatting error that is causing the steps to restart

/remove-label merge-review-in-progress
/remove-label merge-review-needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Typically, we want to refer to "worker" as "compute", unless describing something literal in the code (e.g., a node with role: worker). Seems there is a mix of uses in here, a lot of them probably need to be "worker". Consider adding something that mentions compute as analogous to "worker" near the start since we use "compute" most of the time.

@openshift-ci openshift-ci bot removed merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR labels Oct 14, 2025
@openshift-ci
Copy link

openshift-ci bot commented Oct 14, 2025

@cbippley: all tests passed!

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

@jeana-redhat jeana-redhat added this to the Planned for 4.20 GA milestone Oct 14, 2025
@jeana-redhat jeana-redhat merged commit 4a650bd into openshift:main Oct 14, 2025
2 checks passed
@jeana-redhat
Copy link
Contributor

/cherrypick enterprise-4.20

@openshift-cherrypick-robot

@jeana-redhat: new pull request created: #100493

Details

In response to this:

/cherrypick enterprise-4.20

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.20 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants