Add Virtual Hardware tab to Create VM Wizard #3237

glekner · 2019-11-05T13:00:51Z

Changes

Created a new Virtual Hardware tab for attaching CD-ROMs to a VM/Template
Updated DiskModal to support CD-ROM Add/Edit
Removed CD-ROMs from the Disks table (Create VM Storage Tab, Overview Disks tab)
Removed redundant Type Column from Disks table (Create VM Storage Tab, Overview Disks tab)

matthewcarleton · 2019-11-05T13:47:32Z

@glekner I think this would be similar to what we had for the disk tab where we don't have a specific action for CD-ROMs but incorporate it into the "Add disk" flow. I can put together some designs for it today. I also wonder how this is impacted by advanced > virtual hardware, that could be the right place for this too.

matthewcarleton · 2019-11-07T23:42:46Z

@glekner ya I think we need to determine the priority of the action. I don't see a lot of reasons to add CD-ROMs manually. @jelkosz do you have any thoughts on this? It seems Windows Guest tools are the primary reason we'd see CD-ROMs here. They really are edge cases when we consider there are no physical CD-ROMs to pass through to the VM it's just another PVC.
If we are changing this to add CD-ROMs and Disks I wonder if this should be "Add storage" rather than "Add disk"?
If we are going to show CD-ROMs in the list we might want to call the whole thing just storage and not disks too.

This would indicate there is more than just disks but not highlight CD-ROMs specifically.

atiratree · 2019-12-16T15:47:33Z

frontend/packages/kubevirt-plugin/src/components/modals/cdrom-vm-modal/helpers.ts

+export const getAvailableCDName = (cds: CD[]) => {
+  let index = 1;
+  let name = `cd-drive-${index}`;
+  cds.sort(compareCDS).forEach((cd) => {


I still think #3237 (comment) is a better solution:

Please do not modify the input array with sort as this is an unexpected behaviour for this function

atiratree · 2019-12-19T14:37:12Z

...rt-plugin/src/components/create-vm-wizard/tabs/virtual-hardware-tab/virtual-hardware-tab.tsx

+    bus: DiskBus.VIRTIO,
+  });
+
+  const addButton = () => (


there is no need for this to be a function

atiratree · 2019-12-19T14:39:49Z

.../src/components/create-vm-wizard/tabs/virtual-hardware-tab/vm-wizard-virtualhardware-row.tsx

+    ),
+});
+
+const getActions = (


these actions seem to be the same as the one in vm-wizard-storage-row. Can we export them from there and delete them here?

atiratree · 2019-12-19T14:40:55Z

.../src/components/create-vm-wizard/tabs/virtual-hardware-tab/vm-wizard-virtualhardware-row.tsx

+    <CDSimpleRow
+      data={{ ...restData, name }}
+      validation={{
+        name: validations.name || validations.url || validations.container || validations.pvc,


shouldn't this be content now?

atiratree · 2019-12-19T14:44:21Z

frontend/packages/kubevirt-plugin/src/components/vm-disks/disk-row.tsx

@@ -77,7 +67,7 @@ const menuActionDelete = (
  ),
 });

-const getActions = (
+export const getActions = (


what is this export for?

glekner · 2019-12-22T09:44:32Z

Added isEditing prop to DiskModal usages on disk-row and vm-wizard-storage-row
Using Set now in getAvailableCDName
@suomiy

atiratree · 2020-01-02T15:46:23Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/types.ts

@@ -217,7 +218,7 @@ export type VMWizardStorage = {

 export type VMWizardStorageWithWrappers = VMWizardStorage & {
  diskWrapper: DiskWrapper;


let's make it all optional to be consistent

atiratree · 2020-01-02T15:50:36Z

frontend/.vscode/settings.json

@@ -34,6 +34,9 @@
    "**/bower_components": true,
    "**/.cache-loader": true,
    "**/public/dist": true
+  },


shouldn't this rather go into a separate PR?

atiratree · 2020-01-02T15:52:51Z

two nits, but otherwise looks good

glekner · 2020-01-03T22:57:58Z

done @suomiy

atiratree · 2020-01-06T11:43:08Z

...rt-plugin/src/components/create-vm-wizard/tabs/virtual-hardware-tab/virtual-hardware-tab.tsx

+            <VMCDsTable
+              columnClasses={cdTableColumnClasses}
+              data={virtualStorages}
+              customData={{ isDisabled: disableAddCD, withProgress, removeStorage, wizardReduxID }}


why is the editing and deleting disabled once we reach 2 cds?

@suomiy thats a design decision. A bit controversial I agree, but a reasoning is that it is not needed to have more than 2 - you can have 1 for OS and 1 for drivers but there is no need to have a 3th. ...aaaand we should expose only things which are actually needed...

Ah, sorry, I've misunderstood your comment. Also the delete and edit is disabled here, so it is incorrect I agree. Only the add has to be disabled.

it should still reference isLocked

atiratree · 2020-01-06T14:01:28Z

/lgtm

openshift-ci-robot · 2020-01-06T14:01:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, suomiy

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:

~~frontend/packages/kubevirt-plugin/OWNERS~~ [suomiy]

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

atiratree · 2020-01-06T15:12:23Z

/retest

openshift-bot · 2020-01-06T15:12:50Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-06T15:38:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

glekner · 2020-01-07T12:53:33Z

/retest

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/kubevirt Related to kubevirt-plugin labels Nov 5, 2019

openshift-ci-robot requested review from rawagner and vojtechszocs November 5, 2019 13:01

glekner force-pushed the wizard-attach-cd branch 2 times, most recently from c9cdc67 to 265ae41 Compare November 7, 2019 13:33

irosenzw mentioned this pull request Nov 11, 2019

Bug 1768614: Fix add disk modal #3337

Merged

glekner force-pushed the wizard-attach-cd branch 2 times, most recently from 4829b55 to 4561234 Compare November 19, 2019 11:11

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 19, 2019

glekner force-pushed the wizard-attach-cd branch 3 times, most recently from 0495163 to db2a1a8 Compare November 21, 2019 14:09

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2019

glekner force-pushed the wizard-attach-cd branch from db2a1a8 to d154c8c Compare November 21, 2019 14:12

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2019

glekner force-pushed the wizard-attach-cd branch 4 times, most recently from 05f1719 to 1b00f13 Compare November 21, 2019 15:41

glekner changed the title ~~Attach CD through VM Wizard~~ Add Virtual Hardware tab to Create VM Wizard Nov 21, 2019

glekner force-pushed the wizard-attach-cd branch 4 times, most recently from e260dcc to 4e9afb3 Compare November 28, 2019 13:00

openshift-ci-robot added component/dev-console Related to dev-console component/knative Related to knative-plugin labels Dec 1, 2019

glekner force-pushed the wizard-attach-cd branch 5 times, most recently from f09100a to b4cab28 Compare December 16, 2019 15:14

atiratree reviewed Dec 16, 2019

View reviewed changes

atiratree reviewed Dec 19, 2019

View reviewed changes

glekner force-pushed the wizard-attach-cd branch from b4cab28 to 761d83b Compare December 21, 2019 23:20

glekner force-pushed the wizard-attach-cd branch 2 times, most recently from d770ebd to cac72a8 Compare December 22, 2019 09:49

atiratree reviewed Jan 2, 2020

View reviewed changes

glekner force-pushed the wizard-attach-cd branch from cac72a8 to f2baf11 Compare January 3, 2020 22:57

atiratree suggested changes Jan 6, 2020

View reviewed changes

glekner force-pushed the wizard-attach-cd branch from f2baf11 to c9f463a Compare January 6, 2020 13:31

add virtual-hardware tab to vm-wizard

2eb9b87

glekner force-pushed the wizard-attach-cd branch from c9f463a to 2eb9b87 Compare January 6, 2020 13:59

openshift-ci-robot assigned atiratree Jan 6, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2020

openshift-merge-robot merged commit db156e8 into openshift:master Jan 7, 2020

spadgett added this to the v4.4 milestone Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Virtual Hardware tab to Create VM Wizard #3237

Add Virtual Hardware tab to Create VM Wizard #3237

glekner commented Nov 5, 2019 •

edited

matthewcarleton commented Nov 5, 2019 •

edited

matthewcarleton commented Nov 7, 2019 •

edited

atiratree Dec 16, 2019

atiratree Dec 19, 2019

atiratree Dec 19, 2019

atiratree Dec 19, 2019

atiratree Dec 19, 2019

glekner commented Dec 22, 2019

atiratree Jan 2, 2020

atiratree Jan 2, 2020

atiratree commented Jan 2, 2020

glekner commented Jan 3, 2020

atiratree Jan 6, 2020

jelkosz Jan 6, 2020

jelkosz Jan 6, 2020

glekner Jan 6, 2020

atiratree Jan 6, 2020

atiratree commented Jan 6, 2020

openshift-ci-robot commented Jan 6, 2020

atiratree commented Jan 6, 2020

openshift-bot commented Jan 6, 2020

openshift-bot commented Jan 6, 2020

glekner commented Jan 7, 2020

		@@ -217,7 +218,7 @@ export type VMWizardStorage = {

		export type VMWizardStorageWithWrappers = VMWizardStorage & {
		diskWrapper: DiskWrapper;

Add Virtual Hardware tab to Create VM Wizard #3237

Add Virtual Hardware tab to Create VM Wizard #3237

Conversation

glekner commented Nov 5, 2019 • edited

matthewcarleton commented Nov 5, 2019 • edited

matthewcarleton commented Nov 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glekner commented Dec 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Jan 2, 2020

glekner commented Jan 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Jan 6, 2020

openshift-ci-robot commented Jan 6, 2020

atiratree commented Jan 6, 2020

openshift-bot commented Jan 6, 2020

openshift-bot commented Jan 6, 2020

glekner commented Jan 7, 2020

glekner commented Nov 5, 2019 •

edited

matthewcarleton commented Nov 5, 2019 •

edited

matthewcarleton commented Nov 7, 2019 •

edited