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

CNV-37570: Doc: use pipelines to manage VMs and boot sources #77640

Conversation

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 18, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 18, 2024

@sbeskin-redhat: This pull request references CNV-37570 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

Resolves: https://issues.redhat.com/browse/CNV-37570

OCP 4.16
CNV 4.16

Preview:

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 18, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 20, 2024

@sbeskin-redhat: This pull request references CNV-37570 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

Resolves: https://issues.redhat.com/browse/CNV-37570

OCP 4.16
CNV 4.16

Preview:
https://77640--ocpdocs-pr.netlify.app/openshift-dedicated/latest/virt/virtual_machines/creating_vms_rh/virt-creating-vms-from-instance-types.html#virt-creating-vm-instancetype_virt-creating-vms-from-instance-types

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 openshift-eng/jira-lifecycle-plugin repository.

@yfrimanm
Copy link

Hi @sbeskin-redhat
In the second bullet:

Click Add volume to upload a new volume or use an existing persistent volume claim (PVC), volume snapshot, or data source. Then click Save.

I think we can simplify it and say “Click Add volume to upload a new volume or use an existing persistent volume claim (PVC). Then click Save.”
@dominikholler should we specify that this must be a DataSource?

I also think that this paragraph is too complexed:

If bootable volumes for a certain operating system (Windows, RHEL, or Linux) are missing in the list, this is indicated by the operating system logo that appears under the list. The logo is followed by a suggestion to add a volume for the missing operating system by clicking the Add volume link. In addition, there is a link to the Create a Windows boot source quick start. The same link appears in a popover if you hover the pointer over the question mark icon next to the Select volume to boot from line.

Maybe we can simplify it by saying something like "Logos of operating systems that are not available in the cluster are shown at the bottom of the list to hint the user that they can be added."

@dominikholler
Copy link

I think we can simplify it and say “Click Add volume to upload a new volume or use an existing persistent volume claim (PVC). Then click Save.”

The current doc matches the first line in the Add volume dialog. Technically correct would be
"Click 'Add volume' to upload a new volume or use an existing volume (PVC), snapshot or a containerdisk. Then click Save."
This might be adjusted also in the UI.

@dominikholler should we specify that this must be a DataSource?

The "Add volume" UI will ensure that a DataSource will be created, like expected, no need to explain this to the user.

@yfrimanm
Copy link

@dominikholler I am aware this matches the current text we currently have in the Add volume dialog.
We were thinking to simplify it and make it more user-friendly in the UI as well. WDYT?
cc @ronensdeor

@dominikholler
Copy link

I am aware this matches the current text we currently have in the Add volume dialog.
We were thinking to simplify it and make it more user-friendly in the UI as well. WDYT?

I like to support this idea. Would a generalization/abstraction instead of a list of examples be simpler?

“Click Add volume to use an already existing disk image”

@yfrimanm
Copy link

+1 on generalizing it.
I think we need to make it clear in the sentence that they have 2 options. Maybe something like:
“Click Add volume to upload a new volume or use an existing disk image from the volumes list. Then click Save.”

@sbeskin-redhat
Copy link
Contributor Author

+1 on generalizing it. I think we need to make it clear in the sentence that they have 2 options. Maybe something like: “Click Add volume to upload a new volume or use an existing disk image from the volumes list. Then click Save.”

@yfrimanm Which list are you referring to here? Is it inside the 'Add volume' dialog?

@yfrimanm
Copy link

I meant the "Volumes list" that is shown in the 1st step "Select volume to boot from", but maybe I went too "high-level" here.

@sbeskin-redhat
Copy link
Contributor Author

I meant the "Volumes list" that is shown in the 1st step "Select volume to boot from", but maybe I went too "high-level" here.

@yfrimanm I am a bit confused here. The user has 2 options - to choose a volume from the list displayed in the instanceTypes tab or to click 'Add volume'. If he clicks 'Add volume', the first line of the dialog says: Upload a new volume or use an existing PVC, snapshot or DataSource. Where are those 'existing' volumes appear? Is there an additional list somewhere?

@yfrimanm
Copy link

@sbeskin-redhat from the dialog users can select the source type: they can upload a new volume, they can use existing volume or volume snapshot and they can also import from URL or registry (but I'm not sure we need to mention the last option of the import).
The volumes list is under the "Show all" button, but they can also use the dialog as I mentioned above.

@sbeskin-redhat
Copy link
Contributor Author

sbeskin-redhat commented Jun 25, 2024

@yfrimanm
So when the dialog says 'existing volumes', it refers to the volumes of the list displayed in the instanceTypes tab? How does it make sense? The procedure looked logical up to this point - either you chose a volume from the list or you add a volume. But then, if you choose the option of adding a volume, you are again given the option of choosing a volume from the same list? Or am I missing something?

@ronensdeor
Copy link

ronensdeor commented Jun 25, 2024 via email

@sbeskin-redhat
Copy link
Contributor Author

@ronensdeor Oh, I see, thank you.

@sbeskin-redhat
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Jul 3, 2024
@jldohmann jldohmann added peer-review-in-progress Signifies that the peer review team is reviewing this PR branch/enterprise-4.16 branch/enterprise-4.17 labels Jul 3, 2024
@jldohmann jldohmann added this to the Continuous Release milestone Jul 3, 2024
Copy link
Contributor

@jldohmann jldohmann left a comment

Choose a reason for hiding this comment

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

It's looking good! just some minor comments and suggestions below

modules/virt-creating-vm-instancetype.adoc Outdated Show resolved Hide resolved
modules/virt-creating-vm-instancetype.adoc Outdated Show resolved Hide resolved
modules/virt-creating-vm-instancetype.adoc Outdated Show resolved Hide resolved
@jldohmann jldohmann added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR peer-review-needed Signifies that the peer review team needs to review this PR labels Jul 3, 2024
@sbeskin-redhat sbeskin-redhat force-pushed the CNV-37570_Use_pipelines_to_manage_bootable_volumes branch from 5e165ef to 54bc0f8 Compare July 3, 2024 21:45
@sbeskin-redhat
Copy link
Contributor Author

@jldohmann Thank you for the review!

Copy link

openshift-ci bot commented Jul 3, 2024

@sbeskin-redhat: all tests passed!

Full PR test history. Your PR dashboard.

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.

@sbeskin-redhat
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 Jul 3, 2024
@sheriff-rh sheriff-rh added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Jul 8, 2024
Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

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

Changes LGTM, merging to 4.16+.

@sheriff-rh sheriff-rh merged commit 0945992 into openshift:main Jul 8, 2024
3 checks passed
@sheriff-rh
Copy link
Contributor

/cherrypick enterprise-4.17

@sheriff-rh
Copy link
Contributor

/cherrypick enterprise-4.16

@openshift-cherrypick-robot

@sheriff-rh: new pull request created: #78546

In response to this:

/cherrypick enterprise-4.17

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.

@openshift-cherrypick-robot

@sheriff-rh: new pull request created: #78547

In response to this:

/cherrypick enterprise-4.16

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.

@sbeskin-redhat
Copy link
Contributor Author

@sheriff-rh Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.16 branch/enterprise-4.17 jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. merge-review-in-progress Signifies that the merge review team is reviewing this PR peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants