VirtualMachine details: add Disks tab #1731

atiratree · 2019-06-17T14:18:19Z

Depends on:

screenshot:

openshift-ci-robot · 2019-06-17T14:18:35Z

Hi @suomiy. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

frontend/package.json

frontend/packages/console-shared/src/selectors/index.ts

atiratree · 2019-06-17T14:24:20Z

frontend/packages/kubevirt-plugin/package.json

-    "kubevirt-web-ui-components": "^0.1.34-4"
+    "kubevirt-web-ui-components": "^0.1.34-4",
+    "kubevirt-typescript-api": "^0.18.1-rc",
+    "openshift-typescript-api": "^4.1.0-rc1"


added all TS definitions of openshift/kubevirt objects. These are generated from openapi specifications with few manual changes and released as kubevirt-typescript-api and openshift-typescript-api packages.

maybe it would make sense to move this into main package.json and use all the types across the whole project

atiratree · 2019-06-17T14:26:26Z

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

+import { Firehose, LoadingInline } from '@console/internal/components/utils';
+import { HelpBlock, FormGroup } from 'patternfly-react';
+import { StorageClassModel } from '@console/internal/models';
+import { V1StorageClass } from 'openshift-typescript-api/esm';


eg: usage of the fully typed V1StorageClass

atiratree · 2019-06-17T14:29:57Z

Similar approach will be used for NICs in the followup

atiratree · 2019-06-17T18:21:06Z

I decoupled this PR from #1682. There is just a thin commit which adds YAML editor (b5fec02).

Please let me know if I should make a separate PR for just a YAML editor so that Marek and I can develop in parallel more easily.

@mareklibra

spadgett · 2019-06-17T19:53:19Z

This is not an editor pattern we've used elsewhere in console. Have we considered just showing a dialog? That would be more inline with the existing experience.

atiratree · 2019-06-18T09:57:32Z

created a Base PR for YAML editor and Events: #1739

matthewcarleton · 2019-06-18T12:59:51Z

This is not an editor pattern we've used elsewhere in console. Have we considered just showing a dialog? That would be more inline with the existing experience.

hey @spadgett this design aligns with Patternfly so I believe it's the right way to do it. Does it make sense to have a follow up discussion to determine how to deal with inline editing in OpenShift in general? That might be out of scope for this PR.
https://www.patternfly.org/pattern-library/forms-and-controls/inline-edit/#inline-edit-in-a-table-view

@lizsurette @beanh66 WDYT?

lizsurette · 2019-06-18T15:30:48Z

hey @spadgett this design aligns with Patternfly so I believe it's the right way to do it. Does it make sense to have a follow up discussion to determine how to deal with inline editing in OpenShift in general? That might be out of scope for this PR.
https://www.patternfly.org/pattern-library/forms-and-controls/inline-edit/#inline-edit-in-a-table-view

Agreed that we should probably discuss this as a follow-up. The history here is that within the Create VM flow (which is in a modal wizard) we really couldn't open a modal to edit these items on top of a modal, so went with the inline edit table pattern.

beanh66 · 2019-06-18T17:24:09Z

I agree with @spadgett, we should consider using the kebab menu and popping a modal to make these edits similar to editing labels or annotations today.

lizsurette · 2019-06-18T17:36:04Z

I agree with @spadgett, we should consider using the kebab menu and popping a modal to make these edits similar to editing labels or annotations today.

Are you guys okay with this being inconsistent with the wizard flow of adding/editing Disks and Networks where we will need to use inline tables rather than pop a modal on a modal?

FYI @jelkosz since this would be a new implementation for editing Disks and Networks in VM Details.

jelkosz · 2019-06-19T08:07:37Z

I agree with @spadgett, we should consider using the kebab menu and popping a modal to make these edits similar to editing labels or annotations today.

The annotations editor does not have a specific tab, since there is only an "X annotations" label in the details screen and a pencil next to it. If I click the pencil, a popup shows up which is using the same pattern inside as we have here (e.g. a table where you can edit the fields inline).

I don't think we can have disks and nics to be hidden behind a small "5 disks" label and than having a table in the kebab - configuring virtual machine disks and nics is way too involved operation for that, far from being a list of key-value pairs. Even today, but will grow bigger probably. Also, the disks and nics are the most important HW configurations of the VM...

Anyway, this seems to be a topic where all options have strong pros and cons and we could argue about them for a long time. But the main question for me is, do we want to argue about it now and block this PR? Or can we just proceed with this PR and take the discussion offline to a followup? If the design is at least acceptable, I would really vote against re-writing it now if we want to stood any chance of moving the content of the fork here on time.

atiratree · 2019-06-20T14:49:21Z

removed dependency on the types (#1773)

Can we continue with this PR as is and resolve the ongoing discussion in the followup if the design changes?

matthewcarleton · 2019-06-24T14:09:36Z

I agree with @jelkosz
It would be an odd interaction to have a list of disks that you edit individually in a pop up considering how extensive that edit can possibly be.
We could show this in a modal but it wouldn't be as simple as key value so it's seems like we are forcing a strange experience on the user just to stay in line with other simpler patterns.
This is going to be the same issue for editing NICs so I agree with the general thought here that we should not block this PR but revisit after it is merged to ensure alignment that works across all these scenarios.

jelkosz · 2019-06-26T07:08:56Z

/ok-to-test

...d/packages/kubevirt-plugin/src/components/modals/delete-device-modal/delete-device-modal.tsx

rawagner · 2019-06-26T10:23:27Z

...d/packages/kubevirt-plugin/src/components/modals/delete-device-modal/delete-device-modal.tsx

+
+export type DeleteDeviceModalProps = {
+  deviceType: DeviceType;
+  device: any;


can we add better type ?

The types will be added after #1813 is merged

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

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

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

frontend/packages/kubevirt-plugin/src/k8s/patches/vm-template/index.ts

rawagner · 2019-06-26T10:39:41Z

frontend/packages/kubevirt-plugin/src/selectors/dv/selectors.ts

+
+import { getStorageSize } from '../selectors';
+
+export const getDataVolumeResources = (dataVolume) => _.get(dataVolume, 'spec.pvc.resources');


will be added after #1813

frontend/packages/kubevirt-plugin/src/selectors/pvc/selectors.ts

frontend/packages/kubevirt-plugin/src/selectors/vm/disk.ts

atiratree · 2019-06-26T11:53:23Z

fixed

- actions: create & remove VM disks

rawagner · 2019-06-26T14:09:28Z

/lgtm

openshift-ci-robot · 2019-06-26T14:10:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, 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~~ [rawagner,suomiy]

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

atiratree · 2019-06-26T14:19:07Z

/test backend

atiratree · 2019-06-26T14:19:13Z

/test frontend

atiratree · 2019-06-26T14:23:15Z

/retest

atiratree · 2019-06-26T14:56:41Z

/retest

openshift-bot · 2019-06-26T15:04:01Z

/retest

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

openshift-bot · 2019-06-26T15:54:55Z

/retest

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

openshift-ci-robot requested review from benjaminapetersen and vojtechszocs June 17, 2019 14:18

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 17, 2019

atiratree commented Jun 17, 2019

View reviewed changes

spadgett added this to the v4.2 milestone Jun 17, 2019

atiratree force-pushed the kubevirt.disksPage branch 2 times, most recently from 37dd455 to 6bf96df Compare June 17, 2019 18:16

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 17, 2019

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

atiratree force-pushed the kubevirt.disksPage branch from 6bf96df to 9ef9efc Compare June 18, 2019 16:01

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

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

atiratree force-pushed the kubevirt.disksPage branch from 9ef9efc to 5b86217 Compare June 19, 2019 09:49

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

atiratree force-pushed the kubevirt.disksPage branch from 5b86217 to b10d641 Compare June 19, 2019 10:03

atiratree mentioned this pull request Jun 19, 2019

TypeScript Enhancements #1757

Merged

atiratree force-pushed the kubevirt.disksPage branch from b10d641 to e41c2b3 Compare June 19, 2019 10:51

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

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 20, 2019

atiratree force-pushed the kubevirt.disksPage branch from e425813 to 412e53f Compare June 25, 2019 16:46

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 26, 2019

atiratree force-pushed the kubevirt.disksPage branch 4 times, most recently from 68695af to b101a86 Compare June 26, 2019 09:55

rawagner suggested changes Jun 26, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2019

atiratree force-pushed the kubevirt.disksPage branch 2 times, most recently from a1274b5 to 861aac9 Compare June 26, 2019 11:51

VirtualMachine details: add Disks tab

8594b52

- actions: create & remove VM disks

atiratree force-pushed the kubevirt.disksPage branch from 861aac9 to 8594b52 Compare June 26, 2019 13:58

openshift-ci-robot assigned rawagner Jun 26, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019

openshift-merge-robot merged commit b9bd6db into openshift:master Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VirtualMachine details: add Disks tab #1731

VirtualMachine details: add Disks tab #1731

atiratree commented Jun 17, 2019 •

edited

Loading

openshift-ci-robot commented Jun 17, 2019

atiratree Jun 17, 2019

atiratree Jun 17, 2019

atiratree Jun 17, 2019

atiratree Jun 17, 2019

atiratree commented Jun 17, 2019 •

edited

Loading

atiratree commented Jun 17, 2019 •

edited

Loading

spadgett commented Jun 17, 2019

atiratree commented Jun 18, 2019 •

edited

Loading

matthewcarleton commented Jun 18, 2019

lizsurette commented Jun 18, 2019

beanh66 commented Jun 18, 2019

lizsurette commented Jun 18, 2019

jelkosz commented Jun 19, 2019

atiratree commented Jun 20, 2019

matthewcarleton commented Jun 24, 2019 •

edited

Loading

jelkosz commented Jun 26, 2019

rawagner Jun 26, 2019

atiratree Jun 26, 2019

rawagner Jun 26, 2019

atiratree Jun 26, 2019

atiratree commented Jun 26, 2019

rawagner commented Jun 26, 2019

openshift-ci-robot commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

openshift-bot commented Jun 26, 2019

openshift-bot commented Jun 26, 2019


		import { getStorageSize } from '../selectors';

		export const getDataVolumeResources = (dataVolume) => _.get(dataVolume, 'spec.pvc.resources');

VirtualMachine details: add Disks tab #1731

VirtualMachine details: add Disks tab #1731

Conversation

atiratree commented Jun 17, 2019 • edited Loading

openshift-ci-robot commented Jun 17, 2019

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 Jun 17, 2019 • edited Loading

atiratree commented Jun 17, 2019 • edited Loading

spadgett commented Jun 17, 2019

atiratree commented Jun 18, 2019 • edited Loading

matthewcarleton commented Jun 18, 2019

lizsurette commented Jun 18, 2019

beanh66 commented Jun 18, 2019

lizsurette commented Jun 18, 2019

jelkosz commented Jun 19, 2019

atiratree commented Jun 20, 2019

matthewcarleton commented Jun 24, 2019 • edited Loading

jelkosz commented Jun 26, 2019

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 Jun 26, 2019

rawagner commented Jun 26, 2019

openshift-ci-robot commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

openshift-bot commented Jun 26, 2019

openshift-bot commented Jun 26, 2019

atiratree commented Jun 17, 2019 •

edited

Loading

atiratree commented Jun 17, 2019 •

edited

Loading

atiratree commented Jun 17, 2019 •

edited

Loading

atiratree commented Jun 18, 2019 •

edited

Loading

matthewcarleton commented Jun 24, 2019 •

edited

Loading