Bug 1870114: Updated template PVC name and namespace to use parameters #6428

yaacov · 2020-08-25T12:26:39Z

https://issues.redhat.com/browse/CNV-4764 updated the way PVC name and namespace are stated in the template.
We currently use the old way, we need to update according to latest updates.

Refs:
kubevirt/common-templates#168
kubevirt/kubevirt-ssp-operator#212

openshift-ci-robot · 2020-08-25T12:26:48Z

@yaacov: This pull request references Bugzilla bug 1870114, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1870114: Updated template PVC name and namespace to use parameters

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.

atiratree · 2020-08-25T15:13:18Z

...virt-plugin/src/components/create-vm-wizard/redux/initial-state/storage-tab-initial-state.ts

-      `${TEMPLATE_DATAVOLUME_ANNOTATION}/${relevantOptions.os}`,
-    );
-    const pvcNamespace = iGetAnnotation(iTemplate, `${TEMPLATE_DATAVOLUME_ANNOTATION}/namespace`);
+    const pvcName = iGetPrameter(iTemplate, TEMPLATE_DATAVOLUME_NAME_PARAMETER).value;


is the parameter returned really a plain JS object?

:-) no, I'm fixing that 👍

yaacov · 2020-08-25T17:05:20Z

@suomiy @pcbailey @irosenzw @glekner please review

atiratree · 2020-08-26T06:55:00Z

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

      return {
        id: osId,
        name: getAnnotation(template, nameAnnotation),
-        dataVolumeName: getOperatingSystemDataVolumeAnnotation(template, osId),
-        dataVolumeNamespace: getOperatingSystemDataVolumeNamespaceAnnotation(template),
+        dataVolumeName: getParameter(template, TEMPLATE_DATAVOLUME_NAME_PARAMETER)?.value,


can we have similar selectors and change it to getPrameterValue?

atiratree · 2020-08-26T07:01:00Z

frontend/packages/kubevirt-plugin/src/selectors/immutable/template/combined.ts

@@ -86,6 +86,13 @@ export const iGetRelevantTemplates = (
        const versionCMP = compareVersions(splitVersion(aVersion), splitVersion(bVersion)) * -1; // descending

        if (versionCMP !== 0) {


can we move this logic to compareVersions and also add it to the unit test?

atiratree · 2020-08-26T07:18:09Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

@@ -129,14 +134,16 @@ const baseImageUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions)
    const iCommonTemplates = iGetLoadedCommonData(state, id, VMWizardProps.commonTemplates);
    const iTemplate =
      iCommonTemplates && iGetRelevantTemplate(null, iCommonTemplates, relevantOptions);


can you please include flavor and workloadProfile in hasVMSettingsValueChanged so we always get relevant template here?

ok

p.s.
the spec says that os disk image should not change on cpu/mem or orher none os related changes,
but since we have this problem all over the code, so we can fix it in a folow-up.

it is not an problem as this not about the spec. I'd be glad if our code worked also on a broken/customized env with different templates of different versions with different base images for different oses, if it doesn't cost us that much. Just using these selectors properly with all the relevant info.

atiratree · 2020-08-26T10:14:46Z

frontend/packages/kubevirt-plugin/src/utils/__tests__/sort.spec.ts

+
+  it('check devel version when both of them are not null', () => {
+    expect(compareVersions(osVersion10, osVersion9)).toEqual(1);
+  });
 });

 describe('removeOSDups', () => {


can you please add it to this test as well?

atiratree · 2020-08-26T10:17:48Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

@@ -129,14 +134,16 @@ const baseImageUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions)
    const iCommonTemplates = iGetLoadedCommonData(state, id, VMWizardProps.commonTemplates);
    const iTemplate =
      iCommonTemplates && iGetRelevantTemplate(null, iCommonTemplates, relevantOptions);


it is not an problem as this not about the spec. I'd be glad if our code worked also on a broken/customized env with different templates of different versions with different base images for different oses, if it doesn't cost us that much. Just using these selectors properly with all the relevant info.

atiratree · 2020-08-26T10:19:46Z

frontend/packages/kubevirt-plugin/src/utils/sort.ts

@@ -48,7 +48,7 @@ export const ignoreCaseSort = <T>(
  return array.sort((a, b) => resolve(a).localeCompare(resolve(b)));
 };

-export const splitVersion = (osID: string): number[] =>
+const splitVersion = (osID: string): number[] =>


+1 nice refactoring

atiratree · 2020-08-26T12:32:29Z

/lgtm

openshift-ci-robot · 2020-08-26T12:32:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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,yaacov]

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 · 2020-08-26T23:33:25Z

@yaacov: All pull requests linked via external trackers have merged:

openshift/console#6428

Bugzilla bug 1870114 has been moved to the MODIFIED state.

In response to this:

Bug 1870114: Updated template PVC name and namespace to use parameters

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.

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 25, 2020

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Aug 25, 2020

openshift-ci-robot requested review from rawagner and atiratree August 25, 2020 12:26

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

yaacov changed the title ~~Bug 1870114: Updated template PVC name and namespace to use parameters~~ [WIP] Bug 1870114: Updated template PVC name and namespace to use parameters Aug 25, 2020

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 Aug 25, 2020

atiratree reviewed Aug 25, 2020

View reviewed changes

yaacov force-pushed the updated-the-way-we-collect-pvc-name-and-namespace branch from 2f26c05 to 4df9af0 Compare August 25, 2020 16:42

updated template PVC name and namespace to use parameters

adb17c3

yaacov force-pushed the updated-the-way-we-collect-pvc-name-and-namespace branch from 4df9af0 to adb17c3 Compare August 25, 2020 17:01

yaacov changed the title ~~[WIP] Bug 1870114: Updated template PVC name and namespace to use parameters~~ Bug 1870114: Updated template PVC name and namespace to use parameters Aug 25, 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 Aug 25, 2020

atiratree suggested changes Aug 26, 2020

View reviewed changes

rename getParameter getParamterValue

a5f557c

yaacov force-pushed the updated-the-way-we-collect-pvc-name-and-namespace branch from ad2c586 to a5f557c Compare August 26, 2020 07:45

move devel logic into the compare method

7db54ae

yaacov force-pushed the updated-the-way-we-collect-pvc-name-and-namespace branch from 8936c4a to 7db54ae Compare August 26, 2020 08:02

change os disk image on flavor/wl change

f285fee

atiratree reviewed Aug 26, 2020

View reviewed changes

openshift-ci-robot assigned atiratree Aug 26, 2020

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

openshift-merge-robot merged commit 5b8ab73 into openshift:master Aug 26, 2020

spadgett added this to the v4.6 milestone Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1870114: Updated template PVC name and namespace to use parameters #6428

Bug 1870114: Updated template PVC name and namespace to use parameters #6428

yaacov commented Aug 25, 2020

openshift-ci-robot commented Aug 25, 2020

atiratree Aug 25, 2020

yaacov Aug 25, 2020

yaacov commented Aug 25, 2020

atiratree Aug 26, 2020

yaacov Aug 26, 2020

atiratree Aug 26, 2020

yaacov Aug 26, 2020

atiratree Aug 26, 2020

yaacov Aug 26, 2020

atiratree Aug 26, 2020

atiratree Aug 26, 2020

atiratree Aug 26, 2020

atiratree Aug 26, 2020

atiratree commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020

		@@ -86,6 +86,13 @@ export const iGetRelevantTemplates = (
		const versionCMP = compareVersions(splitVersion(aVersion), splitVersion(bVersion)) * -1; // descending

		if (versionCMP !== 0) {

Bug 1870114: Updated template PVC name and namespace to use parameters #6428

Bug 1870114: Updated template PVC name and namespace to use parameters #6428

Conversation

yaacov commented Aug 25, 2020

openshift-ci-robot commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Aug 25, 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

Choose a reason for hiding this comment

atiratree commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020