Vm overview boot order summary #3560

yaacov · 2019-11-25T11:16:14Z

~~Depend on: #3396~~

Jira: https://jira.coreos.com/browse/CNV-2907

Update boot order summary in VM Overview page.
Design:
https://github.com/openshift/openshift-origin-design/blob/master/web-console/knikubevirt/vm-details/vm-boot-order/vm-boot-order.md

Screenshot:

yaacov · 2019-11-25T11:18:02Z

@suomiy @mareklibra please review

yaacov · 2019-11-25T11:25:49Z

/retest

yaacov · 2019-11-25T11:27:35Z

@Ranelim @matthewcarleton please review.

Ranelim · 2019-11-25T13:30:25Z

On empty state, boot order will mention that no drive was selected, therefore the VM will attempt boot from all the available disks (only disks), by order of appearance on YAML.
This state can occur when a user creates a VM from YAML without providing any priority weight for any drive to boot from.

When there are one or more drives selected on the boot order, the VM will attempt to boot only from the specified (priorities) drives and will disregard the rest of the disks.

https://github.com/openshift/openshift-origin-design/blob/master/web-console/knikubevirt/vm-details/vm-boot-order/vm-boot-order.md

atiratree · 2019-11-25T14:01:22Z

frontend/packages/kubevirt-plugin/src/constants/vm/storage/disk-type.ts

 import { ValueEnum } from '../../value-enum';

 export class DiskType extends ValueEnum<string> {
  static readonly DISK = new DiskType('disk');
  static readonly CDROM = new DiskType('cdrom');
  static readonly FLOPPY = new DiskType('floppy');
  static readonly LUN = new DiskType('lun');
+  static readonly typeLabels = {


this could get confused with the enum: please see #3436 (review)

moved the dictionary away from the consts and renamed it.

also added a constractor option :-)

matthewcarleton · 2019-11-25T15:57:25Z

+1 to what @Ranelim is saying. Also, should we clarify here with "Show default boot disks"?

yaacov · 2019-11-25T16:35:10Z

@Ranelim @matthewcarleton updated the screenshots, please re-review

yaacov · 2019-11-25T18:24:32Z

/test e2e-gcp-console

matthewcarleton · 2019-11-25T18:25:52Z

LGTM @Ranelim WDYT?

yaacov · 2019-11-25T19:52:41Z

/test e2e-gcp-console

yaacov · 2019-11-26T12:00:08Z

/test analyze
/test e2e-gcp-console

atiratree · 2019-11-26T13:37:18Z

frontend/packages/kubevirt-plugin/src/types/types.ts

@@ -11,3 +12,8 @@ export type VMMultiStatus = {
  importerPodsStatuses?: any[];
  progress?: number;
 };
+
+export type BootableDeviceType = {


nit: missing typeLabel

nice ! added

yaacov · 2019-11-26T16:24:28Z

@suomiy @mareklibra rebased, please review.

atiratree · 2019-11-26T18:28:55Z

frontend/packages/kubevirt-plugin/src/components/boot-order/boot-order-summary.tsx

+  };
+
+  // Note(Yaacov): className='text-secondary' is a hack to fix TextVariants being overriden.
+  const EmptyState: React.FC = () => (


can't we make a real component out of this?

atiratree · 2019-11-26T18:29:43Z

frontend/packages/kubevirt-plugin/src/components/boot-order/boot-order-summary.scss

+  margin-top: 0;
+}
+
+.kubevirt-boot-order-suammary__empty-text {


typo: suammary

atiratree · 2019-11-26T18:31:23Z

frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template-resource.tsx

@@ -79,18 +79,14 @@ export const VMTemplateDetailsList: React.FC<VMTemplateResourceListProps> = ({
  canUpdateTemplate,
 }) => {
  const id = getBasicID(template);
-  const sortedBootableDevices = getBootableDevicesInOrder(template);
+  const devices = getDevices(asVM(template));


getDevices already expects VmLikeEntity

🌔 🌃 🕙

atiratree · 2019-11-26T18:31:40Z

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

-    .filter((nic) => nic.bootOrder)
-    .map((nic) => ({ type: DEVICE_TYPE_INTERFACE, value: nic }));
+export const getDevices = (vm: VMLikeEntityKind): BootableDeviceType[] => {
+  const disks = getDisks(asVM(vm)).map((disk) => ({


can we call asVM just once?

atiratree · 2019-11-26T18:38:39Z

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

+    value: disk,
+  }));
+  const nics = getInterfaces(asVM(vm)).map((nic) => ({
+    type: DEVICE_TYPE_INTERFACE,


can we convert these into plain string enum?

yaacov · 2019-11-26T18:46:32Z

/test e2e-gcp-console

yaacov · 2019-11-26T20:49:49Z

/test e2e-gcp-console

matthewcarleton · 2019-11-26T21:03:51Z

frontend/packages/kubevirt-plugin/src/components/boot-order/boot-order-summary-empty-state.tsx

+          isExpanded={isExpanded}
+          className="kubevirt-boot-order-summary__expandable"
+        >
+          <List>


This should be an ordered list but I see that the list component doesn't come with the option. It would be great if we could contribute that back to PatternFly, but in the meantime what's the easiest way to ensure this is numbered? I think it makes a big difference, WDYT @Ranelim (this would apply to the expanded list as well.

Added a commit that use ordered list:

p.s.
@matthewcarleton Will open a PR / Discussion in patternfly's github and cc you.

patternfly/patternfly-react#3356

LGTM @Ranelim WDYT?

Not sure I follow what images I need to update where. The Boot-order PR and the images u added to your PR seem aligned.
But the only difference is how the CD-ROM string appears. Adding @matthewcarleton to confirm this. The design display:
Drive [1]: [container] (CD-COM)
And not:
CD-drive-[1] (CD-ROM)
This is fetched from the CD-COM creation.
The “Drive [#]” is the field title, and the rest is the field name itself.

I like the numbering order implemented. Better than bullets

ya good catch @Ranelim I didn't notice that. I agree we want to keep the proper format for CD-ROMs here.

I think the lack of consistency here is quite unfortunate.

Also it can be quite difficult to understand this constellation:

Drive 2

Drive 1

I think the names would be a better solution here.

@matthewcarleton @Ranelim this PR is continuing in: #3436 please open/continue this discussion there.

#3436 also handle the modal that use the same device labeler, so will also benefit from this discussion.

thanks @yaacov see here

atiratree · 2019-11-27T11:10:01Z

frontend/packages/kubevirt-plugin/src/components/boot-order/boot-order-summary-empty-state.tsx

+
+import './boot-order-summary.scss';
+
+export const BootOrderSummaryEmptyState = ({ devices }: BootOrderSummaryEmptyStateProps) => {


I think we are trying to use BootOrderSummaryEmptyState: React.FC<BootOrderSummaryEmptyStateProps> convention in all of our code

atiratree · 2019-11-27T11:11:33Z

frontend/packages/kubevirt-plugin/src/components/boot-order/boot-order-summary-empty-state.tsx

+
+  const options = devices.filter((device) => !device.value.bootOrder);
+
+  const onToggle = () => {


can you memoize this with useCallback ?

atiratree · 2019-11-27T11:11:53Z

frontend/packages/kubevirt-plugin/src/components/boot-order/boot-order-summary.tsx

+import { BootOrderSummaryEmptyState } from './boot-order-summary-empty-state';
+
+// NOTE(yaacov): using <ol> because '@patternfly/react-core' <List> currently miss isOrder parameter.
+export const BootOrderSummary = ({ devices }: BootOrderSummaryProps) => {


atiratree · 2019-11-27T11:20:43Z

frontend/packages/kubevirt-plugin/src/components/boot-order/boot-order-summary-empty-state.tsx

+        >
+          <ol>
+            {options.map((option) => (
+              <ListItem key={deviceKey(option)}>{deviceLabel(option)}</ListItem>


I would rather use <li> for consistency until the List component is fixed

atiratree · 2019-11-27T11:21:30Z

frontend/packages/kubevirt-plugin/src/components/boot-order/boot-order-summary.tsx

+      ) : (
+        <ol>
+          {sources.map((source) => (
+            <ListItem key={deviceKey(source)}>{deviceLabel(source)}</ListItem>


atiratree · 2019-11-27T14:20:45Z

/approve - without counting the cdrom issue

yaacov · 2019-11-27T15:11:15Z

/lgtm

openshift-ci-robot · 2019-11-27T15:11:16Z

@yaacov: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

glekner · 2019-11-27T15:12:55Z

/lgtm

openshift-ci-robot · 2019-11-27T15:13:09Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

openshift-ci-robot requested review from jelkosz and rhrazdil November 25, 2019 11:16

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/kubevirt Related to kubevirt-plugin labels Nov 25, 2019

yaacov changed the title ~~Vm overview boot order summary~~ [WIP] Vm overview boot order summary Nov 25, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2019

yaacov mentioned this pull request Nov 25, 2019

VM Overview - set boot order #3436

Merged

7 tasks

atiratree reviewed Nov 25, 2019

View reviewed changes

yaacov force-pushed the vm-overview-boot-order-summary branch from cc5545b to b9f2091 Compare November 25, 2019 16:06

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 25, 2019

yaacov force-pushed the vm-overview-boot-order-summary branch from b9f2091 to 49c5597 Compare November 25, 2019 16:36

yaacov force-pushed the vm-overview-boot-order-summary branch from 49c5597 to bf6f0fb Compare November 26, 2019 10:07

yaacov force-pushed the vm-overview-boot-order-summary branch from bf6f0fb to 6a6e92c Compare November 26, 2019 12:24

atiratree reviewed Nov 26, 2019

View reviewed changes

yaacov force-pushed the vm-overview-boot-order-summary branch from be4cb24 to 4157754 Compare November 26, 2019 16:19

vm overview boot order summary

8c6b136

yaacov force-pushed the vm-overview-boot-order-summary branch from 4157754 to 8c6b136 Compare November 26, 2019 16:20

yaacov changed the title ~~[WIP] Vm overview boot order summary~~ Vm overview boot order summary Nov 26, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 26, 2019

atiratree suggested changes Nov 26, 2019

View reviewed changes

yaacov added 2 commits November 26, 2019 21:29

extract empty to a new class

d0671a8

stricter type

abf9fa1

matthewcarleton reviewed Nov 26, 2019

View reviewed changes

use ordered list to show the boot order

0f3c4a9

yaacov force-pushed the vm-overview-boot-order-summary branch from 2c8629f to 0f3c4a9 Compare November 26, 2019 21:54

atiratree suggested changes Nov 27, 2019

View reviewed changes

yaacov added 4 commits November 27, 2019 14:23

fix typing :-)

06fe0f9

use li with ol and not listItem

892b22c

add missing types

63bebab

fix lint

ae1d442

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

openshift-ci-robot assigned glekner Nov 27, 2019

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

openshift-merge-robot merged commit cade0d7 into openshift:master Nov 27, 2019

yaacov mentioned this pull request Dec 4, 2019

Use common method to render the cd summary #3662

Closed


		import './boot-order-summary.scss';

		export const BootOrderSummaryEmptyState = ({ devices }: BootOrderSummaryEmptyStateProps) => {


		const options = devices.filter((device) => !device.value.bootOrder);

		const onToggle = () => {

Vm overview boot order summary #3560

Vm overview boot order summary #3560

Conversation

yaacov commented Nov 25, 2019 • edited

yaacov commented Nov 25, 2019

yaacov commented Nov 25, 2019

yaacov commented Nov 25, 2019

Ranelim commented Nov 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcarleton commented Nov 25, 2019

yaacov commented Nov 25, 2019

yaacov commented Nov 25, 2019

matthewcarleton commented Nov 25, 2019

yaacov commented Nov 25, 2019

yaacov commented Nov 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Nov 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

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

yaacov commented Nov 26, 2019

yaacov commented Nov 26, 2019

matthewcarleton Nov 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ranelim Nov 27, 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

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 Nov 27, 2019

yaacov commented Nov 27, 2019

openshift-ci-robot commented Nov 27, 2019

glekner commented Nov 27, 2019

openshift-ci-robot commented Nov 27, 2019

yaacov commented Nov 25, 2019 •

edited

Ranelim commented Nov 25, 2019 •

edited

matthewcarleton Nov 26, 2019 •

edited

Ranelim Nov 27, 2019 •

edited