Bug 1916888: Modifying OCS wizard Donor chart based on selected device type #7924

Jayashree-panda · 2021-01-26T03:38:50Z

No description provided.

openshift-ci-robot · 2021-01-26T03:38:54Z

@Jayashree-panda: This pull request references Bugzilla bug 1916888, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1916888: Modifying OCS wizard Donor chart based on selected device type

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 · 2021-01-26T03:39:15Z

Hi @Jayashree-panda. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Jayashree-panda · 2021-01-26T03:41:59Z

No description provided.

/bugzilla refresh

openshift-ci-robot · 2021-01-26T03:42:01Z

@Jayashree-panda: This pull request references Bugzilla bug 1916888, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

No description provided.

/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.

afreen23 · 2021-01-27T05:32:36Z

/bugzilla refresh

openshift-ci-robot · 2021-01-27T05:32:40Z

@afreen23: This pull request references Bugzilla bug 1916888, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "4.8.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

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.

afreen23 · 2021-01-27T05:33:17Z

/bugzilla refresh

openshift-ci-robot · 2021-01-27T05:33:22Z

@afreen23: This pull request references Bugzilla bug 1916888, 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.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, 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.

GowthamShanmugam · 2021-01-27T05:39:13Z

LGTM

afreen23 · 2021-01-27T05:36:54Z

.../packages/ceph-storage-plugin/src/components/ocs-install/attached-devices/create-sc/state.ts

@@ -16,7 +16,7 @@ export const initialState: State = {
  showNodesListOnLVS: false,
  diskType: 'All',
  diskMode: diskModeDropdownItems.BLOCK,
-  deviceType: [],
+  deviceType: ['Disk', 'Part'],


Use the deviceTypeDropdownItems

Suggested change

deviceType: ['Disk', 'Part'],

deviceType: [deviceTypeDropdownItems.DISK, deviceTypeDropdownItems.PART],

`

@afreen23 Sure. Will do it.

This is to ensure that the changes remain consistent all over in case of modifications

afreen23

One more, then good to go.

afreen23 · 2021-01-27T06:29:15Z

...ge-plugin/src/components/ocs-install/attached-devices/create-sc/wizard-pages/donut-chart.tsx

+          const hasDeviceType: boolean = state.deviceType.includes(
+            deviceTypeDropdownItems[disk.type.toUpperCase()],


Now when we are storing the values , we can just go with disk.type

Suggested change

const hasDeviceType: boolean = state.deviceType.includes(

deviceTypeDropdownItems[disk.type.toUpperCase()],

const hasDeviceType: boolean = state.deviceType.includes(disk.type)

disk.type has values that are populated using LSO CR, The values are 'part' or 'disk' (small case) and state.deviceType has ['Disk', 'Part'],

Okay, I see the dropdown options are getting set only.
In that case this is good in place for now.

cloudbehl

can you attach a screenshot with and without filter & how it behaves?

cloudbehl · 2021-01-27T06:30:11Z

...ge-plugin/src/components/ocs-install/attached-devices/create-sc/wizard-pages/donut-chart.tsx

+          const hasDeviceType: boolean = state.deviceType.includes(
+            deviceTypeDropdownItems[disk.type.toUpperCase()],
+          );


shouldn't this will work?

Suggested change

const hasDeviceType: boolean = state.deviceType.includes(

deviceTypeDropdownItems[disk.type.toUpperCase()],

);

const hasDeviceType: boolean = state.deviceType.includes(disk.type.toUpperCase());

state.deviceType array has values ['Disk', 'Part'], So disk.type.toUpperCase() wont match('DISK' or 'PART')

It will if you see this change.
https://github.com/openshift/console/pull/7924/files#diff-0b015994d7f949840588912fb313db689a1c818fd5a54eab6804e2cfaf8326f1R20

The value deviceTypeDropdownItems.DISK or deviceTypeDropdownItems.PART is 'Disk' / 'Part' only.

export const deviceTypeDropdownItems = Object.freeze({
DISK: 'Disk',
PART: 'Part',
});

cloudbehl · 2021-01-27T06:30:36Z

...ge-plugin/src/components/ocs-install/attached-devices/create-sc/wizard-pages/donut-chart.tsx

+          if (isValidSize && hasDiskType && hasDeviceType) {
            return true;
          }


Suggested change

if (isValidSize && hasDiskType && hasDeviceType) {

return true;

}

return (isValidSize && hasDiskType && hasDeviceType);

Yes ofcourse. I am making the changes.

Jayashree-panda · 2021-01-27T10:53:25Z

Without filter

With filter

cloudbehl · 2021-01-27T17:57:36Z

have you verified that only those disks/part are used for storage cluster creation not the other ones?

Jayashree-panda · 2021-01-28T04:17:14Z

have you verified that only those disks/part are used for storage cluster creation not the other ones?

What are the other ones you are talking about? Do you mean the other filters like diskType, diskMode etc. ?

Jayashree-panda · 2021-01-28T04:21:39Z

Sure. I am looking on it. Thanks

…

On Thu, Jan 28, 2021 at 9:24 AM Afreen Rahman ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In frontend/packages/ceph-storage-plugin/src/components/ocs-install/attached-devices/create-sc/state.ts <#7924 (comment)>: > @@ -16,7 +17,7 @@ export const initialState: State = { showNodesListOnLVS: false, diskType: 'All', diskMode: diskModeDropdownItems.BLOCK, - deviceType: [], + deviceType: [deviceTypeDropdownItems.DISK, deviceTypeDropdownItems.PART], Use this object instead. https://github.com/openshift/console/blob/ef18881cf0cf1b640a631c9ddc6f2f979309ca67/frontend/packages/local-storage-operator-plugin/src/components/local-volume-set/types.ts#L12-L15 ------------------------------ In frontend/packages/ceph-storage-plugin/src/components/ocs-install/attached-devices/create-sc/wizard-pages/donut-chart.tsx <#7924 (comment)>: > + const hasDeviceType: boolean = state.deviceType.includes( + deviceTypeDropdownItems[disk.type.toUpperCase()], Ah! the object is not helpful here, its for dropdown only. We shall use this one in default state and then it should be fine. https://github.com/openshift/console/blob/ef18881cf0cf1b640a631c9ddc6f2f979309ca67/frontend/packages/local-storage-operator-plugin/src/components/local-volume-set/types.ts#L12-L15 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7924 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH6RVLEEXCLPOMZADOPFW6TS4DNYLANCNFSM4WSYVO3A> .

afreen23

lgtm

afreen23 · 2021-01-28T05:02:25Z

Sure. I am looking on it. Thanks

I would discard that now, since it would require chnages at other places too.
We need to fix i18n as well, so let this change be in first @Jayashree-panda

…
On Thu, Jan 28, 2021 at 9:24 AM Afreen Rahman @.> wrote: @.* requested changes on this pull request. ------------------------------ In frontend/packages/ceph-storage-plugin/src/components/ocs-install/attached-devices/create-sc/state.ts <#7924 (comment)>: > @@ -16,7 +17,7 @@ export const initialState: State = { showNodesListOnLVS: false, diskType: 'All', diskMode: diskModeDropdownItems.BLOCK, - deviceType: [], + deviceType: [deviceTypeDropdownItems.DISK, deviceTypeDropdownItems.PART], Use this object instead.

console/frontend/packages/local-storage-operator-plugin/src/components/local-volume-set/types.ts

Lines 12 to 15 in ef18881

export enum DiskType {

RawDisk = 'disk',

Partition = 'part',

}

------------------------------ In frontend/packages/ceph-storage-plugin/src/components/ocs-install/attached-devices/create-sc/wizard-pages/donut-chart.tsx <#7924 (comment)>: > + const hasDeviceType: boolean = state.deviceType.includes( + deviceTypeDropdownItems[disk.type.toUpperCase()], Ah! the object is not helpful here, its for dropdown only. We shall use this one in default state and then it should be fine.

console/frontend/packages/local-storage-operator-plugin/src/components/local-volume-set/types.ts

Lines 12 to 15 in ef18881

export enum DiskType {

RawDisk = 'disk',

Partition = 'part',

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7924 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AH6RVLEEXCLPOMZADOPFW6TS4DNYLANCNFSM4WSYVO3A .

afreen23 · 2021-01-28T05:05:44Z

have you verified that only those disks/part are used for storage cluster creation not the other ones?

What are the other ones you are talking about? Do you mean the other filters like diskType, diskMode etc. ?

@cloudbehl The change is meant for the donut chart filter only, the cluster creation remains intact.

cloudbehl · 2021-01-28T10:48:20Z

have you verified that only those disks/part are used for storage cluster creation not the other ones?

What are the other ones you are talking about? Do you mean the other filters like diskType, diskMode etc. ?

@cloudbehl The change is meant for the donut chart filter only, the cluster creation remains intact.

RIght, but whatever filter is selected only those disks should be provisioned as PV. So wanted to make sure that we tested it and it's working as expected.

@Jayashree-panda ^^

Jayashree-panda · 2021-01-28T12:14:23Z

@cloudbehl Sure. I will be attaching the screenshots once done with testing.

Jayashree-panda · 2021-01-29T17:22:22Z

The storage cluster gets created based on the filter we select.

cloudbehl · 2021-02-01T05:49:46Z

/ok-to-test

cloudbehl · 2021-02-02T09:33:00Z

/test backend
/lgtm

openshift-ci-robot · 2021-02-02T09:33:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, cloudbehl, Jayashree-panda

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/ceph-storage-plugin/OWNERS~~ [afreen23,cloudbehl]

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 · 2021-02-02T13:44:06Z

@Jayashree-panda: All pull requests linked via external trackers have merged:

openshift/console#7924

Bugzilla bug 1916888 has been moved to the MODIFIED state.

In response to this:

Bug 1916888: Modifying OCS wizard Donor chart based on selected device type

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 bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Jan 26, 2021

openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 26, 2021

openshift-ci-robot added the component/ceph Related to ceph-storage-plugin label Jan 26, 2021

openshift-ci-robot requested review from afreen23 and rawagner January 26, 2021 03:39

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. and removed bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. labels Jan 27, 2021

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 27, 2021

afreen23 suggested changes Jan 27, 2021

View reviewed changes

Jayashree-panda force-pushed the bug-1916888 branch from 0dbb0d6 to 3b94e72 Compare January 27, 2021 06:23

afreen23 suggested changes Jan 27, 2021

View reviewed changes

cloudbehl reviewed Jan 27, 2021

View reviewed changes

updation of donut chart on device type change

9c9aef4

Jayashree-panda force-pushed the bug-1916888 branch from 3b94e72 to 9c9aef4 Compare January 27, 2021 10:43

afreen23 approved these changes Jan 28, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2021

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 1, 2021

openshift-ci-robot assigned cloudbehl Feb 2, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2021

openshift-merge-robot merged commit 3e1fbcb into openshift:master Feb 2, 2021

spadgett added this to the v4.7 milestone Feb 5, 2021

	deviceType: ['Disk', 'Part'],
	deviceType: [deviceTypeDropdownItems.DISK, deviceTypeDropdownItems.PART],

		const hasDeviceType: boolean = state.deviceType.includes(
		deviceTypeDropdownItems[disk.type.toUpperCase()],

Bug 1916888: Modifying OCS wizard Donor chart based on selected device type #7924

Bug 1916888: Modifying OCS wizard Donor chart based on selected device type #7924

Conversation

Jayashree-panda commented Jan 26, 2021

openshift-ci-robot commented Jan 26, 2021

openshift-ci-robot commented Jan 26, 2021

Jayashree-panda commented Jan 26, 2021

openshift-ci-robot commented Jan 26, 2021

afreen23 commented Jan 27, 2021

openshift-ci-robot commented Jan 27, 2021

afreen23 commented Jan 27, 2021

openshift-ci-robot commented Jan 27, 2021

GowthamShanmugam commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 left a comment

Choose a reason for hiding this comment

afreen23 Jan 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudbehl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jayashree-panda commented Jan 27, 2021

cloudbehl commented Jan 27, 2021

Jayashree-panda commented Jan 28, 2021

Jayashree-panda commented Jan 28, 2021 via email

afreen23 left a comment

Choose a reason for hiding this comment

afreen23 commented Jan 28, 2021 • edited

afreen23 commented Jan 28, 2021

cloudbehl commented Jan 28, 2021

Jayashree-panda commented Jan 28, 2021 • edited

Jayashree-panda commented Jan 29, 2021

cloudbehl commented Feb 1, 2021

cloudbehl commented Feb 2, 2021

openshift-ci-robot commented Feb 2, 2021

openshift-ci-robot commented Feb 2, 2021

afreen23 Jan 27, 2021 •

edited

afreen23 commented Jan 28, 2021 •

edited

Jayashree-panda commented Jan 28, 2021 •

edited