kubevirt: simplify wrapper classes #4321

atiratree · 2020-02-14T13:41:19Z

I tried to move most of the duplicate logic to the parent classes and simplify the usage of the wrappers so it is less confusing.

Please let me know, what else would you fix, or name differently.

PS look into commit messages for details.

@vojtechszocs @irosenzw @glekner @pcbailey @mareklibra
@rawagner @yaacov

atiratree · 2020-02-14T13:43:04Z

frontend/packages/kubevirt-plugin/src/k8s/wrapper/vm/disk-wrapper.ts

-export class MutableDiskWrapper extends DiskWrapper {
-  public constructor(disk?: V1Disk, copy = false) {
-    super(disk, { copy });
+  protected sanitize(type: DiskType, { bus }: CombinedTypeData) {


each wrapper should implement its own sanitize function

each wrapper should implement its own sanitize function

In case each wrapper's sanitize will be different in some (wrapper-specific) way, maybe we should make it an abstract method - assuming we'd never want to use the fallback "clone deep" implementation for any of the wrappers.

I would keep for now

we can default to it in the child class

could be used by a type which would have the same attributes for each instance

OK, let's keep it for now.

atiratree · 2020-02-14T13:44:12Z

frontend/packages/kubevirt-plugin/src/k8s/wrapper/common/object-with-type-property-wrapper.ts

-    const type = this.getType();
-    this.setType(type, { ...this.getTypeData(type), ...newTypeData });
-  };
+  public appendType = (type?: TYPE, newTypeData?: COMBINED_TYPE_DATA, sanitize = true) =>


which will be used by default when changing the typeData

atiratree · 2020-02-14T13:44:52Z

frontend/packages/kubevirt-plugin/src/k8s/wrapper/vm/disk-wrapper.ts

+    new DiskWrapper({
+      name,
+      bootOrder,
+    }).setType(type, { bus: bus?.getValue() });


used every time when intializing data from UI

atiratree · 2020-02-14T13:46:56Z

frontend/packages/kubevirt-plugin/src/k8s/wrapper/common/k8s-resource-wrapper.ts

+    return (this as any) as SELF;
+  };
+
+  setName = (name: string) => {


K8sResourceObjectWithTypePropertyWrapper and K8sResourceWrapper cannot be united so we have to duplicate code here. But it should be in only these two places and the duplicated logic is pretty simple

Class hierarchies impose limits on reusability from inheritance perspective.

The closest common ancestor of K8sResourceWrapper and K8sResourceObjectWithTypePropertyWrapper is Wrapper. But IIUC it doesn't make sense to put these methods into Wrapper.

One way to reuse duplicated code here would be the concept of mixins. (Not a big deal if the logic is simple enough.)

yes, let's keep this in mind and take care of it if the logic becomes too advanced or diverges in the future

atiratree · 2020-02-26T13:55:56Z

/retest

atiratree · 2020-02-26T15:26:11Z

/retest

glekner · 2020-02-27T10:01:46Z

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

-      const diskWrapper = DiskWrapper.initialize(disk);
-      const volumeWrapper = VolumeWrapper.initialize(volume);
-      const dataVolumeWrapper = dataVolume && new MutableDataVolumeWrapper(dataVolume, true);
+      const diskWrapper = new DiskWrapper(disk);


i like this syntax 🎉

glekner · 2020-02-27T10:07:08Z

...gin/src/components/create-vm-wizard/redux/stateUpdate/vmSettings/storage-tab-state-update.ts

@@ -26,15 +26,15 @@ export const prefillInitialDiskUpdater = ({ id, prevState, dispatch, getState }:
  const oldType =
    oldSourceStorage &&
    StorageUISource.fromTypes(
-      VolumeWrapper.initialize(oldSourceStorage.volume).getType(),
-      DataVolumeWrapper.initialize(oldSourceStorage.dataVolume).getType(),
+      new VolumeWrapper(oldSourceStorage.volume).getType(),


do we really need to initiate two constructors just for types?

Wrappers encapsulate the type mapping which can be described by any part of the object. So far, it is necessary.

StorageUISource is the mapping of the types for our final custom UI type

glekner · 2020-02-27T10:23:13Z

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

+        volume={new VolumeWrapper(volumeWrapper, true)}
+        dataVolume={dataVolumeWrapper && new DataVolumeWrapper(dataVolumeWrapper, true)}
+        persistentVolumeClaim={
+          persistentVolumeClaimWrapper &&


can we move this safety to the constructor somehow? (applicable to all constructors)

not really, because we need to preserve the information that the value is missing entirely and not just that the value is not filled (datavolume object exists).

I already tried that with empty objects and it was causing more problems than it solved

This also creates an expectation that some wrappers might be falsy (undefined) so code working with them needs to keep that in mind.

yes, the code does

glekner · 2020-02-27T10:23:37Z

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

@@ -58,7 +58,7 @@ const getVirtualStoragesData = (
        volumeWrapper,
        dataVolumeWrapper,
        persistentVolumeClaimWrapper:
-          persistentVolumeClaimWrapper || (pvc && PersistentVolumeClaimWrapper.initialize(pvc)),
+          persistentVolumeClaimWrapper || (pvc && new PersistentVolumeClaimWrapper(pvc)),


same safety comment

#4321 (comment)

atiratree · 2020-03-02T17:19:04Z

rebased

atiratree · 2020-03-02T17:22:04Z

/remove-label needs-rebase

openshift-ci-robot · 2020-03-02T17:22:37Z

@suomiy: The label(s) /remove-label needs-rebase cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash

In response to this:

/remove-label needs-rebase

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.

pcbailey · 2020-03-04T03:29:07Z

frontend/packages/kubevirt-plugin/src/k8s/wrapper/vm/volume-wrapper.ts

+    if (type === VolumeType.DATA_VOLUME) {
+      return { name };
+    }
+    if (type === VolumeType.PERSISTENT_VOLUME_CLAIM) {


Is there a particular reason there's no space between these two conditionals? If not, could you please add one for consistency?

thanks! this made me realize that switch statements are better choice

pcbailey · 2020-03-04T13:17:55Z

frontend/packages/kubevirt-plugin/src/k8s/wrapper/vm/data-volume-wrapper.ts

+    switch (type) {
+      case DataVolumeSourceType.HTTP:
+        return { url };
+      case DataVolumeSourceType.PVC:
+        return { name, namespace };
+      case DataVolumeSourceType.BLANK:
+      default:
+        return {};


+1 - I definitely prefer the switch statements over multiple ifs in this situation.

pcbailey · 2020-03-05T01:18:00Z

frontend/packages/kubevirt-plugin/src/k8s/wrapper/vm/disk-wrapper.ts

@@ -3,53 +3,33 @@ import { V1Disk } from '../../../types/vm/disk/V1Disk';
 import { DiskType, DiskBus } from '../../../constants/vm/storage';

 type CombinedTypeData = {
-  bus?: DiskBus;
+  bus?: string;


Just curious, but why did you drop to a less specific type here?

because we use the raw data in the internal structure of the Wrapper which is string. The handling of the CombinedTypeData and sanitize function were moved to the baseClass. The DiskWrapper can still expose the bus as DiskBus enum in the getter

pcbailey · 2020-03-05T01:27:43Z

/lgtm

atiratree · 2020-03-05T18:06:23Z

/hold cancel

atiratree · 2020-03-05T18:06:54Z

fixed

- unify mutable and immutable constructors - make constructors easier to use - make copy on costruct and asResource more permissive - remove unnecesary mergeWrappers - deprecate asMutableResource and replace with asResource

- deprecate initiliaze functions in favor of new keyword - deprecate storage mutable wrappers - create K8sResourceObjectWithTypePropertyWrapper for multiple inheritance - make constructors easier to use - make copy on costruct and asResource more permissive - deprecate asMutableResource and replace with asResource

…meClaimWrapper - deprecate inner usage of EMPTY objects

- deprecate defaultMergeWrappersWithType as all entities should use mergeWith - simplify initializeFromSimpleData - enhance ObjectWithTypePropertyWrapper and move setters and sanitize functionality there - each wrapper implements its own sanitize function - smiplify constructors and deprecate initializeWithType - constructor can copy wrapper

- remove unnecessary public modifiers

atiratree · 2020-03-06T11:01:42Z

rebased

atiratree · 2020-03-06T11:30:47Z

/retest

atiratree · 2020-03-06T13:34:34Z

/retest

vojtechszocs · 2020-03-06T16:34:07Z

@suomiy Were there any effective changes added recently, or is it just a rebase?

vojtechszocs · 2020-03-06T16:34:48Z

OK, I see there's an extra commit 68708dd - ack 😃

vojtechszocs · 2020-03-06T16:39:20Z

/lgtm

openshift-ci-robot · 2020-03-06T16:40:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pcbailey, suomiy, vojtechszocs

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,vojtechszocs]

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

atiratree · 2020-03-06T21:34:50Z

/retest

openshift-ci-robot · 2020-03-06T21:55:01Z

@suomiy: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/verify	`0e84122`	link	`/test verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

atiratree · 2020-03-06T22:13:30Z

/retest

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/kubevirt Related to kubevirt-plugin labels Feb 14, 2020

openshift-ci-robot requested review from jelkosz and yaacov February 14, 2020 13:42

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

atiratree commented Feb 14, 2020

View reviewed changes

atiratree force-pushed the kubevirt.wizardRefactor branch from 398a66f to 0e84122 Compare February 18, 2020 20:18

glekner reviewed Feb 27, 2020

View reviewed changes

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

atiratree force-pushed the kubevirt.wizardRefactor branch from 0e84122 to 3db7052 Compare March 2, 2020 17:10

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

atiratree force-pushed the kubevirt.wizardRefactor branch from 3db7052 to c50ec9d Compare March 3, 2020 20:33

pcbailey reviewed Mar 4, 2020

View reviewed changes

pcbailey reviewed Mar 5, 2020

View reviewed changes

openshift-ci-robot assigned pcbailey Mar 5, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 5, 2020

atiratree force-pushed the kubevirt.wizardRefactor branch from eb8ef94 to 2bf924b Compare March 5, 2020 11:24

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 5, 2020

atiratree force-pushed the kubevirt.wizardRefactor branch 2 times, most recently from 0674eb6 to 97643aa Compare March 5, 2020 18:05

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

suomiy added 9 commits March 6, 2020 11:56

simplify nic-modal and wrapper classes

c8d3a79

- unify mutable and immutable constructors - make constructors easier to use - make copy on costruct and asResource more permissive - remove unnecesary mergeWrappers - deprecate asMutableResource and replace with asResource

kubevirt: remove default values from ensurePath calls

10194d8

kubevirt: fix types

0a338df

kubevirt: rename ValueEnum to ObjectEnum

7956332

create mergeWith method and deprecate mergeWrappers in PersistentVolu…

4e3aede

…meClaimWrapper - deprecate inner usage of EMPTY objects

kubevirt: add mergeWith to Wrapper

47771dc

kubevirt: replace ifs with switch statements in wrapper's sanitize

68708dd

- remove unnecessary public modifiers

atiratree force-pushed the kubevirt.wizardRefactor branch from 97643aa to 68708dd Compare March 6, 2020 10:58

openshift-ci-robot assigned vojtechszocs Mar 6, 2020

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

openshift-merge-robot merged commit b4e8a69 into openshift:master Mar 7, 2020

spadgett added this to the v4.5 milestone Mar 9, 2020

atiratree mentioned this pull request Mar 25, 2020

kubevirt: move and refactor v2v requests from kubevirt-web-ui-components #4750

Merged

kubevirt: simplify wrapper classes #4321

kubevirt: simplify wrapper classes #4321

Conversation

atiratree commented Feb 14, 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

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 Feb 26, 2020

atiratree commented Feb 26, 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

Choose a reason for hiding this comment

atiratree Mar 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Mar 2, 2020

atiratree commented Mar 2, 2020

openshift-ci-robot commented Mar 2, 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

pcbailey commented Mar 5, 2020

atiratree commented Mar 5, 2020

atiratree commented Mar 5, 2020

atiratree commented Mar 6, 2020

atiratree commented Mar 6, 2020

atiratree commented Mar 6, 2020

vojtechszocs commented Mar 6, 2020

vojtechszocs commented Mar 6, 2020 • edited

vojtechszocs commented Mar 6, 2020

openshift-ci-robot commented Mar 6, 2020

atiratree commented Mar 6, 2020

openshift-ci-robot commented Mar 6, 2020 • edited

atiratree commented Mar 6, 2020

atiratree Mar 5, 2020 •

edited

vojtechszocs commented Mar 6, 2020 •

edited

openshift-ci-robot commented Mar 6, 2020 •

edited