Show only the allowed storage interfaces on Disk-modal #4028

irosenzw · 2020-01-21T16:30:29Z

Fixes: CNV-2914

Enforce the validations from common and user templates
so the user can choose only from the allowed values in the Disk-modal
at the storage tab.

Depends on: #3909

Signed-off-by: Ido Rosenzwig irosenzw@redhat.com

irosenzw · 2020-01-22T09:10:24Z

/retest

atiratree

doesn't work with justWarning validations
doesn't work well with CDs - no errors + no possibility to edit
there should be a different default picked for new disks if VirtIO is not present in allowedBuses
no validation for already created VMs (just a reminder for a followup)

atiratree · 2020-01-22T10:55:22Z

.../kubevirt-plugin/src/components/create-vm-wizard/redux/validations/storage-tab-validation.ts

can we have something akin to iGetRelevantTemplate - getTemplateValidations and getTemplateValidation ?

and call console.warn when there are multiple of them - as that should not happen?

atiratree · 2020-01-22T10:56:09Z

...evirt-plugin/src/components/create-vm-wizard/redux/validations/vm-settings-tab-validation.ts

we could call this after the first if

atiratree · 2020-01-22T11:01:39Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal-enhanced.tsx

there is no need for the spread here

atiratree · 2020-01-22T11:02:21Z

...plugin/src/components/create-vm-wizard/tabs/storage-tab/vm-wizard-storage-modal-enhanced.tsx

we could use getTemplateValidation here as well

atiratree · 2020-01-22T11:02:43Z

...plugin/src/components/create-vm-wizard/tabs/storage-tab/vm-wizard-storage-modal-enhanced.tsx

still probably better check this is not null

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

atiratree · 2020-01-22T11:28:31Z

frontend/packages/kubevirt-plugin/src/utils/validations/common.ts

what happens when allowedBusses become null here?

atiratree · 2020-01-22T11:30:16Z

frontend/packages/kubevirt-plugin/src/utils/validations/common.ts

also you could use joinGrammaticallyListOfItems

atiratree · 2020-01-22T11:32:12Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

why dont we use this? and also pass validation to FormRow?

atiratree · 2020-01-22T11:36:03Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

I suppose the Invalid is good as well, but can we also add isDisabled ?

probably the validation + isDisabled should be enough

atiratree · 2020-01-22T17:30:16Z

.../kubevirt-plugin/src/components/create-vm-wizard/redux/validations/storage-tab-validation.ts

the default is all right? Let's rather do this

Suggested change

const allowedBusses = templateValidation ? templateValidation.getAllowedBusses() : null;

const allowedBusses = (templateValidation || new TemplateValidations()).getAllowedBusses();

atiratree · 2020-01-22T17:34:15Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/selectors/immutable/vm-settings.ts

let's move these to ../template.ts ? as these do not return immutable objects

atiratree · 2020-01-22T17:35:45Z

...plugin/src/components/create-vm-wizard/tabs/storage-tab/vm-wizard-storage-modal-enhanced.tsx

we can do the same trick here as well

(templateValidations || new TemplateValidations()).getAllowedBusses()

atiratree · 2020-01-22T17:36:28Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal-enhanced.tsx

probably not here though as this is just the modal

lets remove this line and let DiskModal take care of it for now

First, this is just for now, there will be a followup. Second, this is DiskModal prop, it has to get the default value.

fair enough, let's do it in a followup. Anyway the DiskModal could contain the default value by itself

atiratree · 2020-01-22T17:38:33Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

easier to read

Suggested change

: Array.from(allowedBusses)[0]),

: [...allowedBusses][0]),

atiratree · 2020-01-22T17:42:09Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

can we do the same thing here and just map the thing without any conditions?

Suggested change

{DiskBus.getAll().map((b) =>

{(allowedBusses || new Set(DiskBus.getAll())).map((b) =>

we probably should check if allowedBusses are null in the begiining and assing new Set(DiskBus.getAll()) so we can use it everywhere without a fear

frontend/packages/kubevirt-plugin/src/utils/validations/template/interval-validation-result.ts

atiratree · 2020-01-22T17:45:07Z

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

https://github.com/openshift/console/pull/4028/files#r369505305, so you don't have to do it here before the return

atiratree · 2020-01-22T17:46:00Z

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

and you can do

Suggested change

if (allowedBusses.size === 0) {

return new Set(allowedBusses.size === 0 ? DiskBus.getAll() : allowedBusses)

atiratree · 2020-01-22T17:46:51Z

frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts

the disk condition should still be here or we could get stuck in cdroms

what are you suggesting?:

remove the line

keep the line and add a comment above for a followup

comment the line and add a comment about a followup

atiratree · 2020-01-22T17:50:24Z

frontend/packages/kubevirt-plugin/src/utils/validations/common.ts

please call toString here

atiratree · 2020-01-22T17:55:16Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

please pass validation to FormRow as it is not visible

atiratree · 2020-01-22T17:55:37Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

IMO in that case we can remove Invalid here as it would be shown twice

atiratree · 2020-01-22T17:57:12Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

sorry it does not work with isDisabled - lets remove it

Are you sure? for me it does:

for me it does not

table:

dialog after opening:

atiratree · 2020-01-23T10:45:56Z

frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts

I meant, lets revert it to the same state it was before. Thumbs up for the TODO.

Suggested change

// TODO: implement CDROM disk bus validation

// TODO: implement CDROM disk bus validation

if (disk.getType() === DiskType.DISK) {

addRequired(allowedBusses.has(disk.getDiskBus()));

validations.diskInterface = validateBus(disk.getDiskBus(), allowedBusses);

}

atiratree · 2020-01-23T12:28:25Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

really no need for this check - it will always return true

atiratree · 2020-01-23T12:29:51Z

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

Ok, I suppose the user should know it is not selectable

We could try to make the disabled work in the future

atiratree · 2020-01-23T12:31:23Z

frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts

ow sorry it should just read:

Suggested change

addRequired(allowedBusses.has(disk.getDiskBus()));

addRequired(disk.getDiskBus());

this just checks the value is not null

atiratree · 2020-01-23T12:33:02Z

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

please move this above the method as this talks about the whole thing

atiratree · 2020-01-23T12:49:08Z

works great!

/lgtm

now let's proceed with the folowups :)

should work with justWarning validations as well
should work with and validate CDs + possibility to edit them
should validater already created VMs
should automatically fix our provision source Disks which are created automatically by the wizard

Fixes: CNV-2914 Enforce the validations from common and user templates so the user can choose only from the allowed values in the Disk-modal at the storage tab. Depends on: openshift#3909 Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

atiratree · 2020-01-23T13:09:29Z

/lgtm

openshift-ci-robot · 2020-01-23T13:09:53Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

irosenzw · 2020-01-23T13:48:55Z

/test analyze

openshift-bot · 2020-01-23T15:09:40Z

/retest

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

benjaminapetersen · 2020-01-23T21:25:03Z

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

+  const templateValidations = getTemplateValidations(state, id);
+  if (templateValidations && templateValidations.length > 0) {
+    templateValidations.length > 1 &&
+      console.warn('WARNING: getTemplateValidation: multiple template validations detected!');


@suomiy @irosenzw
If this is an error that should be handled, ensure there is something in place, otherwise, if its something that is sufficiently handled with the console statement, then please add:

// eslint-disable-next-line no-console console.warn('....');

Otherwise the errors appear every time we build the app.
@jcaianirh

fixed in #3949

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 21, 2020

openshift-ci-robot requested review from mareklibra and yaacov January 21, 2020 16:37

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Jan 21, 2020

irosenzw force-pushed the kubevirt.DiskBusValidation branch from bbbd1fc to 87311ec Compare January 22, 2020 08:00

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

irosenzw force-pushed the kubevirt.DiskBusValidation branch from 87311ec to e25f8a0 Compare January 22, 2020 08:08

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 22, 2020

irosenzw changed the title ~~[WIP] Show only the allowed storage interfaces on Disk-modal~~ Show only the allowed storage interfaces on Disk-modal Jan 22, 2020

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 Jan 22, 2020

atiratree suggested changes Jan 22, 2020

View reviewed changes

irosenzw force-pushed the kubevirt.DiskBusValidation branch 2 times, most recently from f5d75d8 to 02b6ecc Compare January 22, 2020 17:12

atiratree reviewed Jan 22, 2020

View reviewed changes

frontend/packages/kubevirt-plugin/src/utils/validations/common.ts Outdated

Copy link

Member

atiratree Jan 22, 2020

Choose a reason for hiding this comment

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

please call toString here

atiratree reviewed Jan 22, 2020

View reviewed changes

irosenzw force-pushed the kubevirt.DiskBusValidation branch from 02b6ecc to 7c6ab42 Compare January 22, 2020 19:45

atiratree reviewed Jan 23, 2020

View reviewed changes

irosenzw force-pushed the kubevirt.DiskBusValidation branch from 7c6ab42 to 11960d5 Compare January 23, 2020 12:25

atiratree reviewed Jan 23, 2020

View reviewed changes

irosenzw force-pushed the kubevirt.DiskBusValidation branch from 11960d5 to 874fcbe Compare January 23, 2020 12:38

openshift-ci-robot assigned atiratree Jan 23, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 23, 2020

irosenzw force-pushed the kubevirt.DiskBusValidation branch from 874fcbe to aaa0c77 Compare January 23, 2020 13:07

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

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

openshift-merge-robot merged commit 492960d into openshift:master Jan 23, 2020

benjaminapetersen reviewed Jan 23, 2020

View reviewed changes

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

	const allowedBusses = templateValidation ? templateValidation.getAllowedBusses() : null;
	const allowedBusses = (templateValidation \|\| new TemplateValidations()).getAllowedBusses();

	{DiskBus.getAll().map((b) =>
	{(allowedBusses \|\| new Set(DiskBus.getAll())).map((b) =>

	if (allowedBusses.size === 0) {
	return new Set(allowedBusses.size === 0 ? DiskBus.getAll() : allowedBusses)

-    // TODO: implement CDROM disk bus validation
+    // TODO: implement CDROM disk bus validation
+    if (disk.getType() === DiskType.DISK) {
+      addRequired(allowedBusses.has(disk.getDiskBus()));
+      validations.diskInterface = validateBus(disk.getDiskBus(), allowedBusses);
+    }

	addRequired(allowedBusses.has(disk.getDiskBus()));
	addRequired(disk.getDiskBus());

Show only the allowed storage interfaces on Disk-modal #4028

Show only the allowed storage interfaces on Disk-modal #4028

Uh oh!

Conversation

irosenzw commented Jan 21, 2020

Uh oh!

irosenzw commented Jan 22, 2020

Uh oh!

atiratree left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree left a comment •

edited

Loading

atiratree Jan 23, 2020 •

edited

Loading