-
Notifications
You must be signed in to change notification settings - Fork 1.9k
OTA-472: Reorganizing update book and removing redundancy #36015
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.
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
Conversation
|
✔️ Deploy Preview for osdocs ready! 🔨 Explore the source changes: ea69dee 🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/6177551b0131ac00076512c2 😎 Browse the preview: https://deploy-preview-36015--osdocs.netlify.app |
4167b24 to
823ba16
Compare
|
@LalatenduMohanty this is ready for review. Could you also suggest someone from QE to look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we make hardcoded 4.8 for instance in 4.6, 4.7 and 4.8? It would be better to have 4.6, 4.7, 4.8 instance in 4.6, 4.7, 4.8 page respectively.
For instance, {product-title} 4.8 upgrade channels recommend upgrades to 4.8 and upgrades within 4.8. They also recommend upgrades within 4.7 and from 4.7 to 4.8, to allow clusters on 4.7 to eventually upgrade to 4.8.
It's a bit complex. If it's what you want to express, how about changing it to 4.8 upgrade channels recommend upgrades from 4.7 to 4.7, from 4.7 to 4.8, from 4.8 to 4.8?
This strategy ensures that administrators explicitly decide to upgrade to the next minor version of {product-title}.
It also lets the admin decide the next z-stream version, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LalatenduMohanty - Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the existing text. Also 4.8 upgrade channels recommend upgrades from 4.7 to 4.7, from 4.7 to 4.8, from 4.8 to 4.8 gives the impression that 4.8 channel can be used for 4.7 z stream updates which is not correct. The reason we have some 4.7 releases in 4.8 channels because we want to provide edges so that clusters eventually move to 4.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LalatenduMohanty Any suggestions on the text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oc adm upgrade channel is an ability of 4.9 oc binary. It's not applicable to 4.6, 4.7, 4.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shellyyang1989 This content was just moved around so this content has been in our docs already, not newly added. I can do another PR to remove this content in areas that it's not applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's appropriate to have this content in the main branch. It will just have to be manually removed from any backports to 4.8 or earlier (and Git cherry-pick conflicts will make it hard to forget ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking Any suggestion on the text?
cc @jiajliu |
|
@shellyyang1989 had already started some work on the pr, she will continue the review later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command gets messed. Prior to 4.9, we can use oc patch clusterversion version --type json -p '[{"op": "add", "path": "/spec/channel", "value": "<channel>”}]' to change the channel. With 4.9 and later, we can use oc adm upgrade channel <channel> to change the channel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LalatenduMohanty @wking - Do we want to make this update with this PR, or should we create a new PR for these specific changes? If we make the update with this PR, it will cause merge conflicts. However, if we do a new PR for 4.9 then make any other updates for earlier versions, we can mitigate some of that. Since this PR is primarily focused on the reorg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets send a separate PR to update the command for 4.9 and later releases. I can send the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LalatenduMohanty If you could give the lgtm for the changes as it relates to reorganization then I can close this out and we can open PR based on this one, to make any necessary updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sagidlow Sorry for the late review. I think we should fix #36015 (review) as part of this PR. However if you want to handle it separately then I will add a lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's saying A channel can be switched from the web console, but we don't document how to change it from web console. I'd suggest to add the procedure for it.
|
LGTM! I just did a review to make sure everything is building correctly with the re-org, and did not do a comprehensive review because the bulk of the content was not changed in this PR. If you'd like a comprehensive review, please let me know! |
|
@shellyyang1989 - What I'd like to do is create a new PR to address any content issues that there are with these files as you mentioned in the above comments. If we could agree on the reorganized sections that way the content for |
|
LGTM with regard to the re-org. |
LalatenduMohanty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content of updating/updating-cluster.adoc and updating/updating-cluster-between-minor.adoc is exactly same (more or less). Can we fix it as part of this restructure?
87018cd to
8f4b33a
Compare
LalatenduMohanty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current doc for Updating a cluster within a minor version from the web console should be deleted and Updating a cluster between minor versions should be renamed as Updating a cluster within a minor version from the web console
Also I do not see include::modules/update-using-custom-machine-config-pools-canary.adoc[leveloffset=+1] in Updating a cluster within a minor version by using the CLI which needs to be added similar to the newly renamed Updating a cluster within a minor version from the web console
f0fc8f9 to
66359de
Compare
Applies to 4.6+
JIRA Link: https://issues.redhat.com/browse/OTA-472
QE and SME ack required.
Preview Link:
The remaining sections have been updated to remove the redundancy.