Bug 1826234: Filter out duplicate OSes in the new VM wizard #4897

irosenzw · 2020-04-02T14:52:21Z

templates in common-templates have some labels pointing to the same name annotation.
Thus, filter the duplicate OSes names presented in the VM wizard.

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

atiratree

What does it mean, in the future? Shouldn't we commit this change after it is ready?

atiratree · 2020-04-02T15:15:31Z

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

+
+export const removeOSDups = (osArr: OperatingSystemArray): OperatingSystemArray => {
+  const osNames: { [key: string]: string } = {};
+  osArr.forEach((os) => {


what happens when these OS are in different templates?

@suomiy what do you mean?
we expect each record to be in the form: { name: "Fedora31", id: "someID" }.
Am I missing something ?

if you have

template1: { name: "Fedora29+", id: "fedora29" }.

template2 { name: "Fedora29+", id: "fedora31" }.

you will always choose template2 even though you should have chosen template1

* edited

edited original comment

ah I see, I made a mistake - now it makes sense even with mutiple templates - we just choose the newer version one
and if the user wants his to be used he should change their own template to

template1: { name: "Fedora29", id: "fedora29" }.

atiratree · 2020-04-02T15:16:57Z

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

+  const osNames: { [key: string]: string } = {};
+  osArr.forEach((os) => {
+    if (osNames[os.name]) {
+      osNames[os.name] = os.id > osNames[os.name] ? os.id : osNames[os.name];


what happens when the comparison misbehaves? - shouldn't we test at least for numbers?

I checked the comparison with several examples - they all went OK.
@suomiy can you think of a scenario that is would misbehave ?

for example "ubuntu9.10" > "ubuntu10.04"

atiratree · 2020-04-02T15:17:40Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/redux/types.ts

@@ -110,3 +110,8 @@ export type VMSettingsValidationConfig = {
    validator: VmSettingsValidator;
  };
 };
+
+export type OperatingSystemArray = {


please do not create array types, also it should not be under wizard directory

atiratree · 2020-04-02T15:20:28Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx

-      : operatingSystemsNative;
+    const operatingSystems = removeOSDups(
+      openshiftFlag
+        ? ignoreCaseSort(getOperatingSystems(vanillaTemplates, params), ['name'])


is this the only place we should worry about?

@suomiy The template validations are working.
except the wizard, do you think on other places ?

IMO this functionality should be implemented in the following function to be applied everywhere in the wizard (for example this is used in template prefil)

getTemplateOperatingSystems = (templates: TemplateKind[]) in selectors/vm-template/advanced.ts

irosenzw · 2020-04-05T10:19:54Z

@suomiy "in the future" means that the templates in common-templates project are being modified to minimize the Operating-Systems dropdown list by changing the os.template annotations as follows:

instead of

name.os.template.kubevirt.io/fedora29: Fedora 29
name.os.template.kubevirt.io/fedora30: Fedora 30
name.os.template.kubevirt.io/fedora31: Fedora 31

have:

name.os.template.kubevirt.io/fedora29: Fedora 29+
name.os.template.kubevirt.io/fedora30: Fedora 29+
name.os.template.kubevirt.io/fedora31: Fedora 29+

atiratree · 2020-04-06T09:40:40Z

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

+  osArr.forEach((os) => {
+    if (osNames[os.name]) {
+      osNames[os.name] =
+        os.id.toLocaleLowerCase().localeCompare(osNames[os.name].toLocaleLowerCase()) >= 0


better, but still not working

atiratree · 2020-04-06T09:42:07Z

@suomiy "in the future" means that the templates in common-templates project are being modified to minimize the Operating-Systems dropdown list by changing the os.template annotations as follows:

I see, that looks useful

atiratree · 2020-04-20T11:43:38Z

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

+ * return 0 when equal.
+ *
+ */
+const compareVersions = (osVersion1: number[], osVersion2: number[]): number => {


nice function! just few nitpicks

a short test for this function would be also nice

atiratree · 2020-04-20T11:44:03Z

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

+ *
+ */
+const compareVersions = (osVersion1: number[], osVersion2: number[]): number => {
+  if (!osVersion1 || !osVersion2) {


Suggested change

if (!osVersion1 || !osVersion2) {

if (!osVersion1 && !osVersion2) {

should be 0 only in this case no?

otherwise the present one should be larger

atiratree · 2020-04-20T11:47:07Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/native/consts.ts

-export const operatingSystemsNative = [
+import { OperatingSystemRecord } from '../../../types';
+
+export const operatingSystemsNative: OperatingSystemRecord[] = [


could you move this file to src/constants/vm-templates/os.ts please?

atiratree · 2020-04-20T11:55:20Z

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

+      osNames[os.name] = os.id;
+    }
+  });
+  return Object.keys(osNames).map((name) => {


this whole function would be more simpler and readable if we just used sort and then _.sortedUniqBy. It is much easier to add other conditions in the future

openshift-ci-robot · 2020-04-21T09:46:41Z

@irosenzw: This pull request references Bugzilla bug 1826234, 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.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826234: Filter out duplicate OSes in the new VM wizard

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.

irosenzw · 2020-04-21T10:17:16Z

/retest

atiratree · 2020-04-21T11:25:06Z

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

+    return 0;
+  }
+
+  if (!osVersion1) {


these two ifs should just fill osVersion with empty array - you don't know what will come in the other version.

It could be [0] or [null]

can you please add a test for that?

atiratree · 2020-04-21T11:28:43Z

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

+};
+
+const descSortOSes = (os1: OperatingSystemRecord, os2: OperatingSystemRecord): number => {
+  if (os1.name > os2.name) {


can we use localCompare for these two to be more accurate?

atiratree · 2020-04-21T11:33:27Z

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

+};
+
+export const removeOSDups = (osArr: OperatingSystemRecord[]): OperatingSystemRecord[] =>
+  _.orderBy(_.uniqBy(osArr.sort(descSortOSes), 'name'), ['name']);


please can you remove orderBy? This is not the purpose of this function and is sorted by ignoreCaseSort down the road

atiratree · 2020-04-21T11:34:54Z

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

+  return compareVersions(getOSVersion(os1.id), getOSVersion(os2.id)) * -1;
+};
+
+export const removeOSDups = (osArr: OperatingSystemRecord[]): OperatingSystemRecord[] =>


could you add a small test for this function as well?

atiratree · 2020-04-21T11:38:45Z

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

+const getOSVersion = (osName: string): number[] =>
+  osName
+    .split(/\D/)
+    .filter((x) => x)


this should test if it is a number

this would mean that 2.1.0 == 2.0.1

probably makes sense to do the map first

@suomiy I think you are mistaken
2.1.0 will yield [2,1,0]
2.0.1 will yield [2,0,1]
The comparison afterwards will examine the two arrays by the value at the same index,
thus determine that 2.1.0 > 2.0.1

yes, you are right
I forgot that these are strings and not numbers.

"0" == 0 // > true !"0" == !0 // > false

atiratree · 2020-04-21T15:27:53Z

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

+};
+
+export const removeOSDups = (osArr: OperatingSystemRecord[]): OperatingSystemRecord[] =>
+  _.orderBy(_.uniqBy(osArr.sort(descSortOSes), 'name'), ['name']);


Suggested change

_.orderBy(_.uniqBy(osArr.sort(descSortOSes), 'name'), ['name']);

_.orderBy(_.uniqBy([...osArr].sort(descSortOSes), 'name'), ['name']);

better not to change the underlying array we don't own in this function

atiratree · 2020-04-22T07:55:56Z

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

+};
+
+const descSortOSes = (os1: OperatingSystemRecord, os2: OperatingSystemRecord): number => {
+  if (os1.name.localeCompare(os2.name) > 0) {


can we remember result of this call and just check if it is not equal to 0 and return it?

we need to return it multiplied by (-1), but yeah... totally make sense

atiratree · 2020-04-22T07:57:56Z

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

+    return 0;
+  }
+
+  const osVer1 = osVersion1 || [0];


these can be just [] no?

atiratree · 2020-04-22T07:59:39Z

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

@@ -46,3 +47,74 @@ export const ignoreCaseSort = <T>(
  };
  return array.sort((a, b) => resolve(a).localeCompare(resolve(b)));
 };
+
+const getOSVersion = (osName: string): number[] =>
+  osName


Suggested change

osName

(osID || "")

atiratree · 2020-04-22T08:01:03Z

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

+};
+
+const descSortOSes = (os1: OperatingSystemRecord, os2: OperatingSystemRecord): number => {
+  if (os1.name.localeCompare(os2.name) > 0) {


Suggested change

if (os1.name.localeCompare(os2.name) > 0) {

if ((os1.name || "").localeCompare(os2.name) > 0) {

atiratree · 2020-04-22T08:04:44Z

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

+};
+
+export const removeOSDups = (osArr: OperatingSystemRecord[]): OperatingSystemRecord[] =>
+  _.uniqBy([...osArr].sort(descSortOSes), 'name');


Suggested change

_.uniqBy([...osArr].sort(descSortOSes), 'name');

_.uniqBy(osArr.filter(a => a).sort(descSortOSes), 'name');

I guess this makes it more useful and we get a new array as well

In the future, the common-templates will have more labels pointing to the same name annotation. Thus, filter the duplicate OSes names presented in the VM wizard Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

atiratree · 2020-04-22T09:54:04Z

/bugzilla refresh

openshift-ci-robot · 2020-04-22T09:54:07Z

@suomiy: This pull request references Bugzilla bug 1826234, which is valid.

3 validation(s) were run on this bug

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

In response to this:

/bugzilla refresh

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-04-22T09:54:37Z

/lgtm

openshift-ci-robot · 2020-04-22T09:55:02Z

[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

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 · 2020-04-22T11:27:28Z

@irosenzw: All pull requests linked via external trackers have merged: openshift/console#4897. Bugzilla bug 1826234 has been moved to the MODIFIED state.

In response to this:

Bug 1826234: Filter out duplicate OSes in the new VM wizard

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 the component/kubevirt Related to kubevirt-plugin label Apr 2, 2020

openshift-ci-robot requested review from rawagner and atiratree April 2, 2020 14:54

atiratree reviewed Apr 2, 2020

View reviewed changes

irosenzw changed the title ~~CNV-4469: Filter out duplicate OSes in the new VM wizard~~ [WIP] CNV-4469: Filter out duplicate OSes in the new VM wizard Apr 5, 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 Apr 5, 2020

irosenzw force-pushed the os-template-name-change branch from 47dc686 to 0910c0b Compare April 5, 2020 11:30

atiratree reviewed Apr 6, 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 Apr 16, 2020

irosenzw force-pushed the os-template-name-change branch from 0910c0b to 310272b Compare April 19, 2020 13:20

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

irosenzw changed the title ~~[WIP] CNV-4469: Filter out duplicate OSes in the new VM wizard~~ CNV-4469: Filter out duplicate OSes in the new VM wizard Apr 19, 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 Apr 19, 2020

irosenzw force-pushed the os-template-name-change branch from 310272b to 9c59c87 Compare April 19, 2020 14:31

atiratree suggested changes Apr 20, 2020

View reviewed changes

irosenzw force-pushed the os-template-name-change branch from 9c59c87 to e209a29 Compare April 21, 2020 08:41

irosenzw changed the title ~~CNV-4469: Filter out duplicate OSes in the new VM wizard~~ Bug 1826234: Filter out duplicate OSes in the new VM wizard Apr 21, 2020

openshift-ci-robot added bugzilla/unspecified bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 21, 2020

atiratree reviewed Apr 21, 2020

View reviewed changes

irosenzw force-pushed the os-template-name-change branch from e209a29 to c2e88e8 Compare April 22, 2020 07:49

atiratree reviewed Apr 22, 2020

View reviewed changes

irosenzw force-pushed the os-template-name-change branch from c2e88e8 to b64207f Compare April 22, 2020 08:51

CNV-4469: Filter out duplicate OSes in the new VM wizard

8a1212c

In the future, the common-templates will have more labels pointing to the same name annotation. Thus, filter the duplicate OSes names presented in the VM wizard Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

irosenzw force-pushed the os-template-name-change branch from b64207f to 8a1212c Compare April 22, 2020 09:49

openshift-ci-robot assigned atiratree Apr 22, 2020

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

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

openshift-merge-robot merged commit 05d49a6 into openshift:master Apr 22, 2020

spadgett added this to the v4.5 milestone Apr 23, 2020

	if (!osVersion1 \|\| !osVersion2) {
	if (!osVersion1 && !osVersion2) {

	_.orderBy(_.uniqBy(osArr.sort(descSortOSes), 'name'), ['name']);
	_.orderBy(_.uniqBy([...osArr].sort(descSortOSes), 'name'), ['name']);

	if (os1.name.localeCompare(os2.name) > 0) {
	if ((os1.name \|\| "").localeCompare(os2.name) > 0) {

	_.uniqBy([...osArr].sort(descSortOSes), 'name');
	_.uniqBy(osArr.filter(a => a).sort(descSortOSes), 'name');

Bug 1826234: Filter out duplicate OSes in the new VM wizard #4897

Bug 1826234: Filter out duplicate OSes in the new VM wizard #4897

Conversation

irosenzw commented Apr 2, 2020 • edited

atiratree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irosenzw Apr 5, 2020 • edited

Choose a reason for hiding this comment

atiratree Apr 6, 2020 • edited

Choose a reason for hiding this comment

atiratree Apr 6, 2020 • 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

irosenzw Apr 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irosenzw commented Apr 5, 2020

atiratree Apr 6, 2020 • edited

Choose a reason for hiding this comment

atiratree commented Apr 6, 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

openshift-ci-robot commented Apr 21, 2020

irosenzw commented Apr 21, 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

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

openshift-ci-robot commented Apr 22, 2020

atiratree commented Apr 22, 2020

openshift-ci-robot commented Apr 22, 2020

openshift-ci-robot commented Apr 22, 2020

irosenzw commented Apr 2, 2020 •

edited

irosenzw Apr 5, 2020 •

edited

atiratree Apr 6, 2020 •

edited

atiratree Apr 6, 2020 •

edited

irosenzw Apr 5, 2020 •

edited

atiratree Apr 6, 2020 •

edited