Bug 1994586: Create local volume set step says "An error has occurred" #9818

afreen23 · 2021-08-17T13:32:21Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1994586

Before:

After:

Signed-off-by: Afreen Rahman afrahman@redhat.com

openshift-ci · 2021-08-17T13:32:28Z

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

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (ratamir@redhat.com), skipping review request.

In response to this:

Bug 1994586: Create local volume set step says "An error has occurred"

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 · 2021-08-18T14:39:07Z

@afreen23: This pull request references Bugzilla bug 1994586, which is valid.

3 validation(s) were run on this bug

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

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (ratamir@redhat.com), skipping review request.

In response to this:

Bug 1994586: Create local volume set step says "An error has occurred"

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-08-18T15:20:47Z

Forgot to update i18n files. hence tests failing #9818 (comment)

...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx

SanjalKatiyar · 2021-08-18T17:27:48Z

...ate-storage-system/create-storage-system-steps/backing-storage-step/backing-storage-step.tsx

@@ -78,7 +77,6 @@ const StorageClassSelection: React.FC<StorageClassSelectionProps> = ({ dispatch,
        noSelection
        onChange={onStorageClassSelect}
        selectedKey={selected.name}
-        filter={filterSCWithoutNoProv}


Can we now make internal mode cluster with no-prov storage-class as well ??

yeah , we have removed notion of internal and external mode.

...em/create-storage-system-steps/create-local-volume-set-step/create-local-volume-set-step.tsx

...torage-system/create-storage-system-steps/create-local-volume-set-step/selected-capacity.tsx

SanjalKatiyar · 2021-08-18T18:58:10Z

frontend/packages/ceph-storage-plugin/src/components/create-storage-system/footer.tsx

-          await Promise.all(labelNodes(state.capacityAndNodes.nodes));
+        if (nodes) await Promise.all(labelNodes(nodes));
        const requests = payloads.map(({ model, payload }) => k8sCreate(model as K8sKind, payload));
        await Promise.all([labelOCSNamespace(), ...requests]);


Will this: #9409 (comment) be handled in some other PR ?

yeah in PR #9630

SanjalKatiyar

Looks good to me.
/assign @GowthamShanmugam
For one more round of review.

GowthamShanmugam · 2021-08-19T06:59:39Z

...ate-storage-system/create-storage-system-steps/backing-storage-step/backing-storage-step.tsx

+        type: 'wizard/setStepIdReached',
+        payload: 1,
+      });
+    }


I think it's the same without this if condition. Maybe adding this condition will create some confusion while reading the code.

Nope , think of the case when user will move back and forth.
The value for stepIdReached would be more than 1 and then we want this to happen.

It s necessary when user went through a flow but thinks he wants to come back and go with another flow.

I can add this as a comment , to make it clear.

Added a comment

GowthamShanmugam · 2021-08-19T07:25:26Z

...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx

@@ -80,10 +42,15 @@ const SelectCapacityAndNodes: React.FC<SelectCapacityAndNodesProps> = ({
 }) => {
  const { t } = useTranslation();

+  React.useEffect(() => {
+    if (!capacity) dispatch({ type: 'capacityAndNodes/capacity', payload: '2Ti' });


Can do this initialization in initiState itself?

capacityAndNodes: { enableArbiter: false, capacity: '2Ti'', pvCount: 0, },

It was previously but there are two flows - local devices and internal.
In case of local devices we dont want 2Ti. hence initializing states separately in those components

GowthamShanmugam · 2021-08-19T07:29:49Z

...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx

  const onRowSelected = React.useCallback(
    (selectedNodes: NodeKind[]) => {
      const nodesData = createWizardNodeState(selectedNodes);
-      dispatch({ type: 'capacityAndNodes/nodes', payload: nodesData });
+      dispatch({ type: 'wizard/nodes', payload: nodesData });


It will be easy to use if we replace these strings ''wizard/nodes' using constant, it used in multiple places, and a small change in this string will need a lot of effort to modify in all places.

But it's ok, for now, need to do that in the future.

Not really. You will get intellisense for these.
These all are defined as types. Typescript will throw error if you define any string wrong.

console/frontend/packages/ceph-storage-plugin/src/components/create-storage-system/reducer.ts

Lines 233 to 238 in d37cf6e

export type CreateStorageSystemAction =

| { type: 'wizard/setStepIdReached'; payload: number }

| {

type: 'wizard/setStorageClass';

payload: WizardState['storageClass'];

}

GowthamShanmugam · 2021-08-19T07:58:51Z

...ents/create-storage-system/create-storage-system-steps/create-local-volume-set-step/body.tsx

+}
+
+const AllNodesLabel: React.FC<{ count: number }> = ({ count }) => {
+  const { t } = useTranslation();


pass tFunction also as a parameter

There is a hook available so we can use that.
No need to pass t

GowthamShanmugam · 2021-08-19T08:42:42Z

frontend/packages/ceph-storage-plugin/src/components/create-storage-system/reducer.ts

@@ -154,6 +150,8 @@ export type LocalVolumeSet = {
 export const reducer: WizardReducer = (prevState, action) => {
  const newState = _.cloneDeep(prevState);
  switch (action.type) {
+    case 'wizard/setInitialState':


use constant for these

We have types for these. These are consts only.
#9818 (comment)

GowthamShanmugam · 2021-08-19T10:50:58Z

...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx

          <SelectedNodesTable data={nodes} />
-          {!!nodes.length && <NodesDetails nodes={nodes} />}
+          {!!nodes.length && <SelectNodesTableFooter nodes={nodes} />}


you can move this inside <SelectedNodesTable> just like <SelectNodesTable>

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

GowthamShanmugam · 2021-08-19T12:30:08Z

/lgtm

openshift-ci · 2021-08-19T12:33:01Z

[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]
~~frontend/packages/local-storage-operator-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-ci · 2021-08-19T15:03:53Z

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

openshift/console#9818

Bugzilla bug 1994586 has been moved to the MODIFIED state.

In response to this:

Bug 1994586: Create local volume set step says "An error has occurred"

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 bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high 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 17, 2021

openshift-ci bot requested review from cloudbehl and shyRozen August 17, 2021 13:34

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2021

afreen23 force-pushed the 404-message branch from b3948db to 8e7c82e Compare August 18, 2021 14:48

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2021

afreen23 mentioned this pull request Aug 18, 2021

Bug 1988904: Arbiter details not present in ODF wizard #9769

Merged

afreen23 force-pushed the 404-message branch from 8e7c82e to 4f3c6f5 Compare August 18, 2021 15:20