Bug 1811003: Make storage class dropdown required #5071

afreen23 · 2020-04-16T10:10:00Z

Merge after #5017
https://bugzilla.redhat.com/show_bug.cgi?id=1811003

openshift-ci-robot · 2020-04-16T10:10:08Z

@afreen23: This pull request references Bugzilla bug 1811003, 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 ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1811003: Make storage class dropdown required

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.

gnehapk · 2020-04-16T10:14:52Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/create-form.tsx

Why do you need catch block here. handlePromise does handle that.

Because handlePromise returns a promise and it needs to be either returned or caught.
I removed return as you can compare so I need to put catch.

It does not have to be. (remove the return). We can let it be

This is not a new style I am introducing in the console. There is a precedent for it.

When the error happens , the browser logs following error, as the returned error is not caught anywhere
Thats why I changed and just added catch. return can be omitted.

cloudbehl · 2020-04-16T10:13:01Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/create-form.tsx

Suggested change

throw new Error('No storage class is selected');

throw new Error('No storageclass selected');

I found this better.

cloudbehl · 2020-04-16T11:27:27Z

@afreen23 we need it for the add capacity flow as well.

openshift-ci-robot · 2020-04-16T13:51:06Z

@afreen23: This pull request references Bugzilla bug 1811003, 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)

Details

In response to this:

Bug 1811003: Make storage class dropdown required

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.

cloudbehl · 2020-04-16T14:12:03Z

...packages/ceph-storage-plugin/src/components/modals/add-capacity-modal/add-capacity-modal.tsx

I think this is still required if something else fails.

This is all handled by withHandlePromise HOC.

You are right, I got confused with other component.
Added back.

bipuladh · 2020-04-16T20:06:34Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/create-form.tsx

Suggested change

handlePromise(makeOCSRequest(selectedNodes, storageClass, osdSize))

.then(() => {

history.push(

`/k8s/ns/${ns}/clusterserviceversions/${appName}/${referenceForModel(

OCSServiceModel,

)}/${getName(ocsRequestData)}`,

);

})

.catch(() => null);

// eslint-disable-next-line promise/catch-or-return

handlePromise(makeOCSRequest(selectedNodes, storageClass, osdSize))

.then(() => {

history.push(

`/k8s/ns/${ns}/clusterserviceversions/${appName}/${referenceForModel(

OCSServiceModel,

)}/${getName(ocsRequestData)}`,

);

});

bipuladh · 2020-04-16T20:09:57Z

...packages/ceph-storage-plugin/src/components/modals/add-capacity-modal/add-capacity-modal.tsx

This code looks wrong to me. handlePromise does not expose setError or setProgress to the user. There is a bigger issue here.
/hold

i think you missed these lines https://github.com/openshift/console/pull/5071/files#diff-ecff7031320d48d9e801ba99a2fb7188R38-R39

Yes, that's where the bug is. So handlePromise will return props.errorMessage and props.isLoading. These values are different from the state values declared in the line you pointed me to. Hence the handling done by handlePromise is not used here.

You are not manually setting the progress like the code that you just deleted. Hence loading state and error will not be correctly reflected for ALL cases. Technically the deleted code is incorrect as it does not use handlePromise in the correct manner, but mixing the right way and the wrong way causes issues.

Okay then @cloudbehl point makes sense, sorry I missed that in haste.
https://github.com/openshift/console/pull/5071/files#r409588659

bipuladh · 2020-04-16T20:31:16Z

...packages/ceph-storage-plugin/src/components/modals/add-capacity-modal/add-capacity-modal.tsx

Suggested change

.handlePromise(k8sPatch(OCSServiceModel, ocsConfig, [patch]))

.k8sPatch(OCSServiceModel, ocsConfig, [patch]))

No need to use handlePromise since the props provided by it is not used anywhere in the code.

- Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1811003 Signed-off-by: Afreen Rahman <afrahman@redhat.com>

afreen23 · 2020-04-16T21:11:59Z

/hold cancel

bipuladh · 2020-04-16T22:13:29Z

/lgtm
Looks great!

openshift-ci-robot · 2020-04-16T22:13:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, bipuladh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/packages/ceph-storage-plugin/OWNERS~~ [afreen23,bipuladh]

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

bipuladh · 2020-04-16T22:16:47Z

/retest

openshift-ci-robot · 2020-04-17T01:28:28Z

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

Details

In response to this:

Bug 1811003: Make storage class dropdown required

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/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 16, 2020

openshift-ci-robot added the component/ceph Related to ceph-storage-plugin label Apr 16, 2020

openshift-ci-robot requested review from gnehapk and shyRozen April 16, 2020 10:10

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

gnehapk reviewed Apr 16, 2020

View reviewed changes

cloudbehl reviewed Apr 16, 2020

View reviewed changes

afreen23 force-pushed the make-sc-dropdown-required branch from 5df89b0 to 1b2122a Compare April 16, 2020 10:36

afreen23 force-pushed the make-sc-dropdown-required branch 2 times, most recently from e99ab89 to d906718 Compare April 16, 2020 13:49

cloudbehl reviewed Apr 16, 2020

View reviewed changes

bipuladh reviewed Apr 16, 2020

View reviewed changes

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

afreen23 force-pushed the make-sc-dropdown-required branch from d906718 to 87b6c02 Compare April 16, 2020 20:29

bipuladh reviewed Apr 16, 2020

View reviewed changes

Storage class should be selected in install form

b13121a

- Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1811003 Signed-off-by: Afreen Rahman <afrahman@redhat.com>

afreen23 force-pushed the make-sc-dropdown-required branch from 87b6c02 to b13121a Compare April 16, 2020 21:10

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

openshift-ci-robot assigned bipuladh Apr 16, 2020

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

openshift-merge-robot merged commit 5f713fd into openshift:master Apr 17, 2020

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

	throw new Error('No storage class is selected');
	throw new Error('No storageclass selected');

-    handlePromise(makeOCSRequest(selectedNodes, storageClass, osdSize))
-      .then(() => {
-        history.push(
-          `/k8s/ns/${ns}/clusterserviceversions/${appName}/${referenceForModel(
-            OCSServiceModel,
-          )}/${getName(ocsRequestData)}`,
-        );
-      })
-      .catch(() => null);
+    // eslint-disable-next-line promise/catch-or-return
+    handlePromise(makeOCSRequest(selectedNodes, storageClass, osdSize))
+      .then(() => {
+        history.push(
+          `/k8s/ns/${ns}/clusterserviceversions/${appName}/${referenceForModel(
+            OCSServiceModel,
+          )}/${getName(ocsRequestData)}`,
+        );
+      });

	.handlePromise(k8sPatch(OCSServiceModel, ocsConfig, [patch]))
	.k8sPatch(OCSServiceModel, ocsConfig, [patch]))

Bug 1811003: Make storage class dropdown required #5071

Bug 1811003: Make storage class dropdown required #5071

Conversation

afreen23 commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afreen23 Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bipuladh Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afreen23 Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudbehl commented Apr 16, 2020

Uh oh!

openshift-ci-robot commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afreen23 commented Apr 16, 2020

Uh oh!

bipuladh commented Apr 16, 2020

Uh oh!

openshift-ci-robot commented Apr 16, 2020

Uh oh!

bipuladh commented Apr 16, 2020

Uh oh!

openshift-ci-robot commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

afreen23 commented Apr 16, 2020 •

edited

Loading

afreen23 Apr 16, 2020 •

edited

Loading

bipuladh Apr 16, 2020 •

edited

Loading

afreen23 Apr 16, 2020 •

edited

Loading