Skip to content

Conversation

@bgaydosrh
Copy link

@bgaydosrh bgaydosrh commented Aug 25, 2022

For 4.12 only.

Jira: https://issues.redhat.com/browse/CNV-17539

Direct doc preview link: https://49607--docspreview.netlify.app/openshift-enterprise/latest/virt/about-virt.html

Tagging Alex Kalenyuk for Code Review in Jira.
Tagging Jenia Peimer for QE Review. in Jira.

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 25, 2022
@bgaydosrh bgaydosrh force-pushed the CNV-17539 branch 2 times, most recently from 68bd380 to aee748d Compare August 25, 2022 16:39
Copy link

@akalenyu akalenyu left a comment

Choose a reason for hiding this comment

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

I think we should bring up assisted-installer here, would that be reasonable?
Just because the main focus of the epic was that
The assisted installer is now providing a new out-of-the-box storage solution.

Up until 4.11, selecting CNV on SNO meant that we also auto deploy the hostpath provisioner storage solution. Now with 4.12, we have LVMO instead (you could still use HPP).

More info can be found here:
https://issues.redhat.com/browse/CNV-17537?focusedCommentId=20783231&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-20783231

@bgaydosrh bgaydosrh force-pushed the CNV-17539 branch 2 times, most recently from 80e9ae0 to b53cd36 Compare September 8, 2022 20:27
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2022
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Sep 21, 2022

🤖 Updated build preview is available at:
https://49607--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/2673

@bgaydosrh bgaydosrh force-pushed the CNV-17539 branch 2 times, most recently from 08253d6 to cdf11b0 Compare September 29, 2022 19:02
@bgaydosrh
Copy link
Author

@akalenyu - This PR should be ready for final review. See the Jira link above for more details. Thanks.

Copy link

@akalenyu akalenyu left a comment

Choose a reason for hiding this comment

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

Thank you! I think we are really close to getting the point across

@bgaydosrh bgaydosrh force-pushed the CNV-17539 branch 2 times, most recently from 86f4423 to 6fd0f85 Compare October 11, 2022 19:52
@bgaydosrh
Copy link
Author

Thank you! I think we are really close to getting the point across

Thanks @akalenyu - let me know what you think of this version and we can send it to QE.

@akalenyu
Copy link

Thank you! I think we are really close to getting the point across

Thanks @akalenyu - let me know what you think of this version and we can send it to QE.

/lgtm
Thank you!

@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2022

@akalenyu: changing LGTM is restricted to collaborators

Details

In response to this:

Thank you! I think we are really close to getting the point across

Thanks @akalenyu - let me know what you think of this version and we can send it to QE.

/lgtm
Thank you!

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.

@bgaydosrh bgaydosrh force-pushed the CNV-17539 branch 2 times, most recently from 240ba52 to c82882f Compare October 19, 2022 22:19
@bgaydosrh
Copy link
Author

bgaydosrh commented Oct 19, 2022

Just FYI @akalenyu - This is transparent to you but I moved the content into the assembly, rather than the module file, as we can't have xrefs in our modules due to an internal limitation. Thanks @ousleyp for catching this......

I will wait for Jenia to QE this. If she can't get to it within the week, I'll see about getting another resource.

Thanks
Bob

@jpeimer
Copy link

jpeimer commented Oct 23, 2022

/lgtm
Thanks @bgaydosrh and @akalenyu!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2022
Copy link
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

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

A few suggestions!

@bergerhoffer bergerhoffer added peer-review-done Signifies that the peer review team has reviewed this PR CNV Label for all CNV PRs branch/enterprise-4.12 labels Oct 25, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2022
@openshift-ci
Copy link

openshift-ci bot commented Oct 26, 2022

New changes are detected. LGTM label has been removed.

@bgaydosrh
Copy link
Author

bgaydosrh commented Oct 26, 2022

@akalenyu - Peer review is complete. I only have one question for you. We cannot use "out-of-the-box" according to our style guidelines, mostly due to translation issues (it's kind of industry slang). Can we say "preconfigured" instead, as in:

"When provisioning with the assisted installer, preconfigured persistent storage is automatically deployed".

@akalenyu
Copy link

@akalenyu - Peer review is complete. I only have one question for you. We cannot use "out-of-the-box" according to our style guidelines, mostly due to translation issues (it's kind of industry slang). Can we say "preconfigured" instead, as in:

"When provisioning with the assisted installer, preconfigured persistent storage is automatically deployed".

Definitely can. thank you

Copy link
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

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

Just a few last things to fix!

Copy link
Contributor

Choose a reason for hiding this comment

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

This now repeats as "Single-node OpenShift OpenShift Container Platform differences". I think you should remove the {product-title} now

Copy link
Author

Choose a reason for hiding this comment

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

Done, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

This now renders as "OpenShift Container Platform Data Foundation Logical Volume Manager Operator", which isn't correct. This one of those cases where it's fine to hard code "OpenShift" because it's part of the Operator name: "OpenShift Data Foundation Logical Volume Manager Operator"

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

an -> a

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@bgaydosrh bgaydosrh force-pushed the CNV-17539 branch 2 times, most recently from 6e6b281 to 852c196 Compare October 27, 2022 23:35
@bgaydosrh
Copy link
Author

@bergerhoffer - This should be ready for merge/review. Thanks!

Copy link
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

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

LGTM for merging!

@bergerhoffer bergerhoffer merged commit 2573411 into openshift:main Oct 31, 2022
@bergerhoffer
Copy link
Contributor

/cherrypick enterprise-4.12

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #52325

Details

In response to this:

/cherrypick enterprise-4.12

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.

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

Labels

branch/enterprise-4.12 CNV Label for all CNV PRs peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants