Bug 1910483: Fix for Storage cluster creation failed with 'Cannot read property "protocol" of null ' on adding& then cancelling Encryption #7688

GowthamShanmugam · 2020-12-31T11:48:01Z

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1910483

Signed-off-by: Gowtham Shanmugasundaram gshanmug@redhat.com

openshift-ci-robot · 2020-12-31T11:48:22Z

Hi @GowthamShanmugam. 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.

cloudbehl · 2021-01-04T05:23:11Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

+    if(!checked){
+      encryptOj.advanced = false;


why you want to set advance encryption to true when checked is set to false?

I am setting advance encryption false only when Encryption is unchecked or set to false

afreen23

/ok-to-test
Nit: Some naming suggestions.

afreen23 · 2021-01-04T05:18:10Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

@@ -76,10 +76,14 @@ export const EncryptionFormGroup: React.FC<EncryptionFormGroupProps> = ({
  }, [encryption.clusterWide, encryption.storageClass, encryptionChecked]);

  const toggleEncryption = (checked: boolean) => {
-    setEncryptionDispatch(ActionType.SET_ENCRYPTION, mode, dispatch, {
+    const encryptOj = {


Suggested change

const encryptOj = {

const payload = {

afreen23 · 2021-01-04T05:18:29Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

-    });
+    }
+    if(!checked){
+      encryptOj.advanced = false;


Suggested change

encryptOj.advanced = false;

payload.advanced = false;

afreen23 · 2021-01-04T05:19:24Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

+    if(!checked){
+      encryptOj.advanced = false;
+    }
+    setEncryptionDispatch(ActionType.SET_ENCRYPTION, mode, dispatch, encryptOj);


Suggested change

setEncryptionDispatch(ActionType.SET_ENCRYPTION, mode, dispatch, encryptOj);

setEncryptionDispatch(ActionType.SET_ENCRYPTION, mode, dispatch, payload);

afreen23 · 2021-01-04T07:32:40Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

-    });
+    }
+    if(!checked){
+      payload.advanced = false;


We should also set all encryption level and both encryption and kms states to initial value when the high level encryption box is unchecked.
This is to ensure that stale states dont cause errors,

payload.advanced = false; payload.storageClass = false; payload.hasHandled = false;

There is already one check for clearing kms state on L88, please merge the above with this one.

console/frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

Lines 88 to 91 in c29d749

if (!checked) {

setEncryptionDispatch(ActionType.CLEAR_KMS_STATE, mode, dispatch);

}

};

afreen23 · 2021-01-04T08:52:09Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

      ...encryption,
      clusterWide: checked,
-    });
+    }


Suggested change

}

};

You are getting formatting errors hence tests are failing:

/go/src/github.com/openshift/console/frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx 82:6 error Insert `;` prettier/prettier 83:7 error Replace `(!checked)` with `·(!checked)·` prettier/prettier

Let me check this

afreen23 · 2021-01-05T05:20:07Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

    if (!checked) {
+      payload.advanced = false;
+      payload.hasHandled = false;


That seems weird but it is used for validation and need to be set to true

Suggested change

payload.hasHandled = false;

payload.hasHandled = true;

I dont understand what validation, let me check the code again

The initial state sets "hasHandled" to true, lets follow the same defaults.

afreen23

Squash the commits as well.

GowthamShanmugam · 2021-01-05T05:55:27Z

@afreen23 test case passed, Please review

cloudbehl · 2021-01-06T06:01:16Z

@GowthamShanmugam can you squash the commits.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1910483 Signed-off-by: Gowtham Shanmugasundaram <gshanmug@redhat.com>

GowthamShanmugam · 2021-01-06T07:30:07Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-wizard/configure.tsx

@@ -76,14 +76,17 @@ export const EncryptionFormGroup: React.FC<EncryptionFormGroupProps> = ({
  }, [encryption.clusterWide, encryption.storageClass, encryptionChecked]);


@afreen23 I can see a separate useEffect is to set "hasHandled" flag for all cases, Setting a flag is kind of redundant and leads to unpredicted behavior, so I removed it from my latest change

GowthamShanmugam · 2021-01-06T08:14:39Z

@cloudbehl @afreen23 all changes are done, please review

afreen23 · 2021-01-07T05:47:25Z

/lgtm

openshift-ci-robot · 2021-01-07T05:47:54Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

openshift-bot · 2021-01-07T10:41:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-01-11T13:17:01Z

@GowthamShanmugam: Bugzilla bug 1910483 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

Bug 1910483: Fix for Storage cluster creation failed with 'Cannot read property "protocol" of null ' on adding& then cancelling Encryption

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 · 2021-01-25T05:58:44Z

/Bugzilla refresh

openshift-ci-robot · 2021-01-25T05:58:46Z

@cloudbehl: Bugzilla bug 1910483 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

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.

cloudbehl · 2021-01-25T05:59:46Z

/Bugzilla refresh

openshift-ci-robot · 2021-01-25T05:59:50Z

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

Bugzilla bug 1910483 has been moved to the MODIFIED state.

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.

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

openshift-ci-robot requested review from afreen23 and rawagner December 31, 2020 11:48

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 31, 2020

cloudbehl reviewed Jan 4, 2021

View reviewed changes

afreen23 suggested changes Jan 4, 2021

View reviewed changes

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 Jan 4, 2021

afreen23 suggested changes Jan 4, 2021

View reviewed changes

GowthamShanmugam force-pushed the bz_1910483 branch 3 times, most recently from cf01817 to 1416c31 Compare January 4, 2021 18:32

afreen23 suggested changes Jan 5, 2021

View reviewed changes

afreen23 reviewed Jan 5, 2021

View reviewed changes

Fix for Storage creation failed with 'Cannot read "protocol" of null'

b60f62e

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1910483 Signed-off-by: Gowtham Shanmugasundaram <gshanmug@redhat.com>

GowthamShanmugam force-pushed the bz_1910483 branch from 1416c31 to b60f62e Compare January 6, 2021 07:17

GowthamShanmugam commented Jan 6, 2021

View reviewed changes

openshift-ci-robot assigned afreen23 Jan 7, 2021

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

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

openshift-merge-robot merged commit 08ea464 into openshift:master Jan 7, 2021

spadgett added this to the v4.7 milestone Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1910483: Fix for Storage cluster creation failed with 'Cannot read property "protocol" of null ' on adding& then cancelling Encryption #7688

Bug 1910483: Fix for Storage cluster creation failed with 'Cannot read property "protocol" of null ' on adding& then cancelling Encryption #7688

GowthamShanmugam commented Dec 31, 2020

openshift-ci-robot commented Dec 31, 2020

cloudbehl Jan 4, 2021

GowthamShanmugam Jan 4, 2021

afreen23 left a comment

afreen23 Jan 4, 2021

GowthamShanmugam Jan 4, 2021

afreen23 Jan 4, 2021

GowthamShanmugam Jan 4, 2021

afreen23 Jan 4, 2021

GowthamShanmugam Jan 4, 2021

afreen23 Jan 4, 2021

GowthamShanmugam Jan 4, 2021

afreen23 Jan 4, 2021

afreen23 Jan 4, 2021

GowthamShanmugam Jan 4, 2021

afreen23 Jan 5, 2021

GowthamShanmugam Jan 5, 2021 •

edited

afreen23 Jan 5, 2021

afreen23 left a comment

GowthamShanmugam commented Jan 5, 2021

cloudbehl commented Jan 6, 2021

GowthamShanmugam Jan 6, 2021 •

edited

GowthamShanmugam commented Jan 6, 2021

afreen23 commented Jan 7, 2021

openshift-ci-robot commented Jan 7, 2021

openshift-bot commented Jan 7, 2021

openshift-ci-robot commented Jan 11, 2021

cloudbehl commented Jan 25, 2021

openshift-ci-robot commented Jan 25, 2021

cloudbehl commented Jan 25, 2021

openshift-ci-robot commented Jan 25, 2021

	setEncryptionDispatch(ActionType.SET_ENCRYPTION, mode, dispatch, encryptOj);
	setEncryptionDispatch(ActionType.SET_ENCRYPTION, mode, dispatch, payload);

	if (!checked) {
	setEncryptionDispatch(ActionType.CLEAR_KMS_STATE, mode, dispatch);
	}
	};

		@@ -76,14 +76,17 @@ export const EncryptionFormGroup: React.FC<EncryptionFormGroupProps> = ({
		}, [encryption.clusterWide, encryption.storageClass, encryptionChecked]);

Bug 1910483: Fix for Storage cluster creation failed with 'Cannot read property "protocol" of null ' on adding& then cancelling Encryption #7688

Bug 1910483: Fix for Storage cluster creation failed with 'Cannot read property "protocol" of null ' on adding& then cancelling Encryption #7688

Conversation

GowthamShanmugam commented Dec 31, 2020

openshift-ci-robot commented Dec 31, 2020

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

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

GowthamShanmugam Jan 5, 2021 • edited

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

GowthamShanmugam commented Jan 5, 2021

cloudbehl commented Jan 6, 2021

GowthamShanmugam Jan 6, 2021 • edited

Choose a reason for hiding this comment

GowthamShanmugam commented Jan 6, 2021

afreen23 commented Jan 7, 2021

openshift-ci-robot commented Jan 7, 2021

openshift-bot commented Jan 7, 2021

openshift-ci-robot commented Jan 11, 2021

cloudbehl commented Jan 25, 2021

openshift-ci-robot commented Jan 25, 2021

cloudbehl commented Jan 25, 2021

openshift-ci-robot commented Jan 25, 2021

GowthamShanmugam Jan 5, 2021 •

edited

GowthamShanmugam Jan 6, 2021 •

edited