kubevirt: refactor createNicRow to NICModal #2670

atiratree · 2019-09-10T21:02:03Z

add edit nic functionality
refactor nic patches (create PatchBuilder utility)

depends on:

kubevirt: add ResultTab to CreateVMWizard #2568

atiratree · 2019-09-10T21:19:12Z

/hold

mareklibra · 2019-10-03T11:38:43Z

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

-export const getVolumes = (vm: VMKind) => _.get(vm, 'spec.template.spec.volumes') || [];
-export const getDataVolumeTemplates = (vm: VMKind) => _.get(vm, 'spec.dataVolumeTemplates') || [];
+export const getDisks = (vm: VMKind, defaultValue = []) =>
+  _.get(vm, 'spec.template.spec.domain.devices.disks') || defaultValue;


Based on recent discussions, we moved from _.get() to direct access like

return vm && vm.spec ? vm.spec.template : undefined;

can we do it consistently in one big PR instead?

The change is simple and quick, much faster than long discussions and PR ping-pongs.
New PRs respect this "new" approach and we should not add new selectors which are not conforming it.
As an independent task, I would batch-change the older code.

changed to new approach

mareklibra · 2019-10-03T11:39:30Z

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

 export const getBootDeviceIndex = (devices, bootOrder) =>
  devices.findIndex((device) => device.bootOrder === bootOrder);
+
+export const getDeviceBootOrder = (device, defaultValue?): number =>
+  _.get(device, 'bootOrder', defaultValue);


direct access (see another comment)

I still think we should not add more lodash-style selectors

mareklibra · 2019-10-03T11:43:24Z

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

+  const devicesWithoutRemovedDevice =
+    removedDeviceName == null
+      ? devices
+      : devices.filter((device) => getSimpleName(device) !== removedDeviceName);


I think simple use of device.name !== removedDeviceName would be more readable

I would leave it for consistency because the same function is used on the device few lines bellow in setListRemove

I agree with the opinion that the amount of indirections for simple things should be reduced. I do not see added value in these simple wrappers.
But let's not block this PR on that.

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

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

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

...packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/result-tab/result-tab-row.scss

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard-footer.tsx

...d/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/result-tab/error-results.tsx

mareklibra · 2019-10-03T12:15:30Z

...packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/result-tab/pending-results.tsx

+      <LoadingInline />
+    </div>
+    <Title headingLevel="h5" size="lg">
+      {makeSentence(`${getVMLikeModelName(isCreateTemplate)} is being created.`)}


The need for makeSentence() can be workarounded by i.e. by staring the sentence with The.

There is no The in the design (http://openshift.github.io/openshift-origin-design/web-console/knikubevirt/Create-vm/step-6-results/wizard-results) so I will leave it in the current state for now.

resolved to a different solution

atiratree · 2019-10-04T15:07:16Z

/retest

atiratree · 2019-10-07T11:45:06Z

/hold cancel

atiratree · 2019-10-07T12:37:24Z

screenshot

atiratree · 2019-10-07T12:37:33Z

/retest

mareklibra · 2019-10-08T06:25:55Z

frontend/packages/kubevirt-plugin/src/hooks/use-show-error-toggler.ts

+
+export const useShowErrorToggler = (
+  initialShowError: boolean = false,
+  initialIsValid: boolean = false,


nit: if a default is set, the param should be optional

default value makes it not optional and initialIsValid? cannot be used

The default value is applied if the parameter is omitted when calling, so it is optional.
If a parameter is not optional, then its value must be provided (either true or false in that case).
But it's not a blocker, imo.

I get that it is actually optional for the caller but not according to typescript (which is probably concerned about the user of the parameter - the function?)

mareklibra · 2019-10-08T06:28:19Z

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

 export const getBootDeviceIndex = (devices, bootOrder) =>
  devices.findIndex((device) => device.bootOrder === bootOrder);
+
+export const getDeviceBootOrder = (device, defaultValue?): number =>
+  _.get(device, 'bootOrder', defaultValue);


I still think we should not add more lodash-style selectors

mareklibra · 2019-10-08T06:32:03Z

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

+};
+
+export type V1Network = {
+  name?: string;


Just a question: Shouldn't there be at least one field which is not optional?

it can be optional when it is initialized internally

- add edit nic functionality - refactor nic patches (create PatchBuilder utility)

mareklibra

Just minor comments which do not block this from merging.

/lgtm

mareklibra · 2019-10-08T10:35:34Z

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

+              isRequired
+              id={asId('name')}
+              value={name}
+              onChange={(v) => setName(v)}


nit:

Suggested change

onChange={(v) => setName(v)}

onChange={setName}

this does not work because the second argument is event and React.useState callback complains about that

mareklibra · 2019-10-08T10:40:25Z

frontend/packages/kubevirt-plugin/src/hooks/use-show-error-toggler.ts

+
+export const useShowErrorToggler = (
+  initialShowError: boolean = false,
+  initialIsValid: boolean = false,


The default value is applied if the parameter is omitted when calling, so it is optional.
If a parameter is not optional, then its value must be provided (either true or false in that case).
But it's not a blocker, imo.

openshift-ci-robot · 2019-10-08T10:43:39Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

openshift-bot · 2019-10-08T11:34:36Z

/retest

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

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 10, 2019

openshift-ci-robot requested review from kyoto and mareklibra September 10, 2019 21:02

openshift-ci-robot added component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared labels Sep 10, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2019

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch from f578fb2 to 3aeb453 Compare September 10, 2019 21:25

atiratree mentioned this pull request Sep 18, 2019

kubevirt: add Networking Tab to CreateVMWizard #2757

Merged

1 task

atiratree changed the base branch from master to master-4.3 September 18, 2019 12:55

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch 5 times, most recently from bcd3d70 to 17ec684 Compare September 23, 2019 10:28

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

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch from 17ec684 to 72b30a6 Compare September 23, 2019 11:32

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

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch from 72b30a6 to 3d1f6b6 Compare September 24, 2019 13:45

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

atiratree changed the base branch from master-4.3 to master September 26, 2019 09:31

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch 6 times, most recently from d0d1f3c to 94b5db7 Compare September 30, 2019 14:39

mareklibra reviewed Oct 3, 2019

View reviewed changes

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch 2 times, most recently from 3eb420f to 1b3b828 Compare October 3, 2019 13:10

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch from 1b3b828 to 9cfc598 Compare October 4, 2019 13:11

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch from 9cfc598 to c9fabc5 Compare October 7, 2019 08:17

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2019

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch from c9fabc5 to 8e2e8fd Compare October 7, 2019 11:46

mareklibra reviewed Oct 8, 2019

View reviewed changes

kubevirt: refactor createNicRow to NICModal

a8928d2

- add edit nic functionality - refactor nic patches (create PatchBuilder utility)

atiratree force-pushed the kubevirt.createVMWizard.nicWizard branch from 8e2e8fd to a8928d2 Compare October 8, 2019 10:28

mareklibra reviewed Oct 8, 2019

View reviewed changes

openshift-ci-robot assigned mareklibra Oct 8, 2019

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

openshift-merge-robot merged commit 96830d4 into openshift:master Oct 8, 2019

spadgett added this to the v4.3 milestone Oct 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubevirt: refactor createNicRow to NICModal #2670

kubevirt: refactor createNicRow to NICModal #2670

atiratree commented Sep 10, 2019 •

edited

atiratree commented Sep 10, 2019

mareklibra Oct 3, 2019

atiratree Oct 3, 2019

mareklibra Oct 8, 2019

atiratree Oct 8, 2019

mareklibra Oct 3, 2019

mareklibra Oct 8, 2019

atiratree Oct 8, 2019

mareklibra Oct 3, 2019

atiratree Oct 3, 2019

mareklibra Oct 8, 2019 •

edited

mareklibra Oct 3, 2019

atiratree Oct 3, 2019

atiratree Oct 4, 2019

atiratree commented Oct 4, 2019

atiratree commented Oct 7, 2019

atiratree commented Oct 7, 2019

atiratree commented Oct 7, 2019

mareklibra Oct 8, 2019

atiratree Oct 8, 2019

mareklibra Oct 8, 2019

atiratree Oct 8, 2019

mareklibra Oct 8, 2019

mareklibra Oct 8, 2019

atiratree Oct 8, 2019

mareklibra left a comment

mareklibra Oct 8, 2019

atiratree Oct 8, 2019

mareklibra Oct 8, 2019

openshift-ci-robot commented Oct 8, 2019

openshift-bot commented Oct 8, 2019

kubevirt: refactor createNicRow to NICModal #2670

kubevirt: refactor createNicRow to NICModal #2670

Conversation

atiratree commented Sep 10, 2019 • edited

atiratree commented Sep 10, 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

Choose a reason for hiding this comment

mareklibra Oct 8, 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

atiratree commented Oct 4, 2019

atiratree commented Oct 7, 2019

atiratree commented Oct 7, 2019

atiratree commented Oct 7, 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

mareklibra left a 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

openshift-ci-robot commented Oct 8, 2019

openshift-bot commented Oct 8, 2019

atiratree commented Sep 10, 2019 •

edited

mareklibra Oct 8, 2019 •

edited