Skip to content
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

[ACM-3650]: Adding KubeVirt as a provider for hosted control planes #4803

Merged
merged 17 commits into from
Apr 19, 2023

Conversation

lahinson
Copy link
Collaborator

@lahinson lahinson commented Mar 8, 2023

This PR adds documentation to the hosted control planes section about using KubeVirt as a provider. This will be Technology Preview for MCE 2.3.

Related Jira task: https://issues.redhat.com/browse/ACM-3650

Copy link

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

here's some preliminary feedback. i'll give this a more detailed look later this week.

Copy link

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

hey, here's some more detailed feedback. I know a lot of the content has been pulled from resources I wrote. Since this is official documentation, we need to mention OpenShift Virtualization as the product that we're supporting hosted control planes with. I tried to sort through that a bit. It's a bit tricky.

@lahinson
Copy link
Collaborator Author

Thanks for your feedback, @davidvossel and @rokej! I made a few updates based on your comments. When you can, let me know what you think.

@lahinson
Copy link
Collaborator Author

I added a list of benefits to the introduction, based on this blog post: https://cloud.redhat.com/blog/effortlessly-and-efficiently-provision-openshift-clusters-with-openshift-virtualization

Copy link

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

I made a couple of more minor comments. I think this is getting close

@lahinson
Copy link
Collaborator Author

@davidvossel I made a few more updates based on your feedback. Let me know what you think when you can!

Copy link

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@lahinson
Copy link
Collaborator Author

@swopebrandi1 Do you have the bandwidth to peer review the new KubeVirt docs for hosted control planes sometime in the next week or so? David and Roke have already reviewed them.

Copy link
Contributor

@swopebe swopebe left a comment

Choose a reason for hiding this comment

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

I asked for a number of changes but will note here some that may apply in various places:

We usually pull out the command from the file that needs to be applied:

  1. Create a __ file similar to the following example, with the .... specification.
  2. Run the oc -apply <file to apply your ___ file.

(Sometimes they are together in these sections)

We remove extra stuff like EOF which is located in a few spots.

(There may be some formatting issues, which are usually caused by an extra asterisks. I cannot easily tell but will check as we get closer to final.)

@lahinson
Copy link
Collaborator Author

Thanks, Brandi! I will take a look at your comments this afternoon.

@chenz4027
Copy link

Hey @lahinson , the dev portion was merged. stolostron/console#2665

I used ${DOC_BASE_PATH}/clusters/index#hosted-control-planes-manage-kubevirt, hope it remains the same here.

@lahinson
Copy link
Collaborator Author

lahinson commented Apr 6, 2023

@chenz4027 That looks right to me. Thanks!

@chenz4027
Copy link

Perfect, thanks @lahinson for the quick confirmation :)

@lahinson
Copy link
Collaborator Author

To do: Incorporate updates from https://github.com/openshift/hypershift/pull/2318/files

Copy link

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

the content looks great. I left one comment about the formatting that i was unsure about

@lahinson
Copy link
Collaborator Author

@swopebe When you're back from PTO, please take a look at the latest draft of the KubeVirt docs and let me know if we're ready to merge. Thank you!

Copy link

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 11, 2023
@openshift-ci openshift-ci bot removed the lgtm label Apr 13, 2023
@openshift-ci openshift-ci bot added the lgtm label Apr 19, 2023
@openshift-ci
Copy link

openshift-ci bot commented Apr 19, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: davidvossel, lahinson, swopebe

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

Needs approval from an approver in each of these files:

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

@lahinson
Copy link
Collaborator Author

Thanks, Brandi!

@openshift-ci
Copy link

openshift-ci bot commented Apr 19, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: davidvossel, lahinson, swopebe

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

Needs approval from an approver in each of these files:

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

@lahinson lahinson merged commit eb70be0 into 2.8_stage Apr 19, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants