-
Notifications
You must be signed in to change notification settings - Fork 231
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
Fix wrong link and typo #778
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: David AUFFRAY <31964077+Davidffry@users.noreply.github.com>
Hi @Davidffry. Thanks for your PR. I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Thanks @Davidffry |
No really reason, but re-read I view, in the header, that there is a note with this "By deploying KubeVirt on top of OpenShift the user can benefit from the OpenShift Template functionality." maybe change into kind : "Only on Openshift Cluster" There is no Template CRD in Kubevirt/Kubernetes deployment. Only Openshift. |
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.
Thanks for the PR @Davidffry !
I have one suggestion to make it a bit easier to parse as having a parenthesis after the "like" makes it seems like the sentence has been cut short.
I see other annotation examples still have it (which we should change in a subsequent PR), but hopefully this makes this one a bit clearer.
@@ -237,7 +237,7 @@ the kubevirt services. | |||
|
|||
Here's a description of the kubevirt annotations. Unless otherwise | |||
specified, the following keys are meant to be top-level entries of the | |||
template metadata, like | |||
template metadata, like (for Openshift console to display) |
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.
template metadata, like (for Openshift console to display) | |
template metadata. For OpenShift console to display: |
PR needs rebase. 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. |
@Davidffry Thanks for the contribution! Can you rebase and address Andrew's comment? Thanks! /lgtm |
/delgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabiand, jean-edouard The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/remove-lgtm Andrew, please lgtm once we are good |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with /lifecycle rotten |
What this PR does / why we need it: Change / Fix / Add feature for documentation
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged): No Issue fixFixes #
Special notes for your reviewer: I think we need to change all the oc command in the documentation, to be more agnostic. If some specificities in Openshift, either create each time if necessary a dedicated chapter or push it outside the kubevirt.
Because you ask to try with the quickstart but we can't use the oc command.
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: