Refactored Node Selection Page for OCS Installation Flow #5017

bipuladh · 2020-04-13T11:36:39Z

Inspired from Adds Local Volume Set Creation Form and enables LSO Plugin #5014
cc @afreen23
/assign @cloudbehl @gnehapk @afreen23

gnehapk · 2020-04-13T11:49:33Z

This looks like a big change to me. We should merge this post baremetal changes as the changes are in same file and can delay the baremetal PR to get merge. PR - https://github.com/openshift/console/pull/5016/files

afreen23 · 2020-04-13T11:42:06Z

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

+          customData={{ selectedNodes, setSelectedNodes, visibleRows, setVisibleRows }}
+        />
+        <p className="control-label help-block" data-test-id="nodes-selected">
+          {selectedNodes?.length ?? 0} node(s) selected


Consider using pluralize here.

Additionally this count can be inside the NodeList component to keep the margin aligned.

afreen23 · 2020-04-13T11:45:49Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

+        nodeObject: node,
+        cpuSpec,
+        memSpec,
+        id: node.metadata.uid,


Custom props can be given under props field.
This will ensure correct typings.

props: { nodeObject: node, cpuSpec, memSpec, id: node.metadata.uid, }

The rows of Pf are of IRow type

{ cells?: (React.ReactNode | IRowCell)[]; isOpen?: boolean; parent?: number; compoundParent?: number; props?: any; fullWidth?: boolean; noPadding?: boolean; heightAuto?: boolean; showSelect?: boolean; isExpanded?: boolean; isFirstVisible?: boolean; isLastVisible?: boolean; selected?: boolean; }

Interesting will do.

afreen23 · 2020-04-13T11:57:05Z

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

+  // intentionally keeping the taint logic as its required in 4.3 and will be handled with checkbox selection
+  // promises.push(...makeTaintNodesRequest(selectedData));


Can be removed now

Done as a part of #5016

cloudbehl · 2020-04-13T17:12:04Z

frontend/packages/ceph-storage-plugin/src/utils/install.ts

Suggested change

return !_.isEmpty(node.spec.taints);

return !_.isEmpty(node.spec?.taints);

gnehapk · 2020-04-15T07:32:55Z

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

Error also needs to be handled here -

.catch((err) => { setProgress(false); setError(err.message); });

handlePromise does this for me.

afreen23 · 2020-04-13T12:28:11Z

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

+          customData={{ selectedNodes, setSelectedNodes, visibleRows, setVisibleRows }}
+        />
+        <p className="control-label help-block" data-test-id="nodes-selected">
+          {selectedNodes?.length ?? 0} node(s) selected


Additionally this count can be inside the NodeList component to keep the margin aligned.

afreen23 · 2020-04-15T06:18:01Z

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

We had this bug fixed in #4676. Please add it.

Suggested change

{(isIndependent === false || mode === MODES.CONVERGED) && (

<CreateOCSServiceForm match={match} />

)}

{(isIndependent === false || mode === MODES.CONVERGED) && clusterServiceVersion (

<CreateOCSServiceForm match={match} />

)}

No need for it.

Since we are not passing it anymore.

afreen23 · 2020-04-15T07:36:59Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

Its no more CustomNodeTable now :)

afreen23 · 2020-04-15T07:38:17Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

You can import the type from patternfly IRow['cells']

afreen23 · 2020-04-15T07:43:37Z

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

Suggested change

const [selectedNodes, setSelectedNodes] = React.useState(null);

const [visibleRows, setVisibleRows] = React.useState(null);

const [selectedNodes, setSelectedNodes] = React.useState<NodeKind[]>([]);

const [visibleRows, setVisibleRows] = React.useState<NodeKind[]>([]);

Initial state as empty array would be better

yes but null is used for handling first render logic.

I can add types though.

afreen23 · 2020-04-15T08:06:53Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

Suggested change

type NodeRow = {

type NodeTableRow = {

afreen23 · 2020-04-15T08:50:36Z

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

All labels should be of same style.

Select Nodes is defined as title IDK if that needs change. For now, I don't want to touch it.

With the updated designs , yes we need to change that .
Refer this: https://marvelapp.com/a3abjjd/screen/66836688

May we can use PF forms here for overall consistency. I think I can send a followup PR for this and the CR changes on top of yours.

@afreen23 that design does not have OCS Service Capacity.

And helper text are not matching. Selected nodes will be labeled with <code>cluster.ocs.openshift.io/openshift-storage=""</code> to create the OCS Service
This one is missing from the design

afreen23 · 2020-04-15T08:54:58Z

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

If you make initial state as an empty array, you can skip this logic.

Suggested change

{`${pluralize(selectedNodes?.length || 0, 'node', 'nodes')} selected`}

{`${pluralize(selectedNodes.length, 'node')} selected`}

mentioned above

afreen23 · 2020-04-15T08:57:06Z

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

Suggested change

<label className="control-label" htmlFor="ocs-service-stoargeclass">

<label className="control-label" htmlFor="ocs-service-capacity">

Add the corresponding matching id to the component

afreen23 · 2020-04-15T09:07:32Z

Can you fix the alignment here and change text to Create Storage Cluster ?

cloudbehl · 2020-04-15T10:51:07Z

frontend/packages/ceph-storage-plugin/src/utils/install.ts

Suggested change

const taints: Taint[] = node.spec.taints || [];

const taints: Taint[] = node.spec?.taints || [];

cloudbehl · 2020-04-15T10:53:32Z

frontend/packages/ceph-storage-plugin/src/constants/ocs-install.ts

use cephStorageLabel from index.ts

line 97 in create-form.tsx use the same variable

cloudbehl · 2020-04-15T10:59:30Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

Suggested change

if (JSON.stringify(filteredData) !== JSON.stringify(visibleRows)) {

if (!_.isEqual(filteredData, visibleRows)) {

cloudbehl · 2020-04-15T11:01:48Z

frontend/packages/ceph-storage-plugin/src/constants/ocs-install.ts

We should take data from Example CSV of storage-cluster rather than using this const.

cloudbehl · 2020-04-15T11:03:43Z

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

Suggested change

const [selectedNodes, setSelectedNodes] = React.useState<NodeKind[]>(null);

const [selectedNodes, setSelectedNodes] = React.useState<NodeKind[]>([]);

Doing this you don't need to do selectedNodes?.length

I need this for the first render. I use it to set the preselection of nodes.

cloudbehl · 2020-04-15T11:06:53Z

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

Let's not hardcode. There is a function which changes / to ~. Let use the function and do string concatenation on OCS_LABEL.

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

afreen23 · 2020-04-15T11:49:19Z

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

With the updated designs , yes we need to change that .
Refer this: https://marvelapp.com/a3abjjd/screen/66836688

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

afreen23 · 2020-04-16T02:32:22Z

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

May we can use PF forms here for overall consistency. I think I can send a followup PR for this and the CR changes on top of yours.

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

afreen23 · 2020-04-16T02:59:56Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

+const NodeTable: React.FC<NodeTableProps> = (props) => {
+  const { selectedNodes, setSelectedNodes, visibleRows } = props.customData;
+
+  const onSelect = (_event, isSelected: boolean, rowIndex: number, rowData: IRowData) => {


Use the onSelect type from PF table props

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

cloudbehl · 2020-04-16T08:31:54Z

/retest

cloudbehl · 2020-04-16T08:48:32Z

/lgtm

openshift-ci-robot · 2020-04-16T08:49:11Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [bipuladh,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 assigned afreen23, cloudbehl and gnehapk Apr 13, 2020

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

openshift-ci-robot requested review from dtaylor113 and gnehapk April 13, 2020 11:37

openshift-ci-robot added the component/core Related to console core functionality label Apr 13, 2020

afreen23 reviewed Apr 13, 2020

View reviewed changes

bipuladh force-pushed the refactor-install branch from f9d4e19 to 813922c Compare April 13, 2020 15:32

cloudbehl reviewed Apr 13, 2020

View reviewed changes

bipuladh force-pushed the refactor-install branch from 813922c to 38cac31 Compare April 14, 2020 19:49

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

bipuladh force-pushed the refactor-install branch from 38cac31 to 65b3a54 Compare April 14, 2020 19:53

bipuladh requested review from afreen23 and cloudbehl April 14, 2020 19:54

gnehapk reviewed Apr 15, 2020

View reviewed changes

afreen23 suggested changes Apr 15, 2020

View reviewed changes

bipuladh force-pushed the refactor-install branch from 65b3a54 to b3c306a Compare April 15, 2020 10:24

bipuladh requested review from afreen23 and gnehapk April 15, 2020 10:28

cloudbehl reviewed Apr 15, 2020

View reviewed changes

bipuladh force-pushed the refactor-install branch 2 times, most recently from 578af65 to 8d8f6a7 Compare April 15, 2020 11:44

bipuladh requested a review from cloudbehl April 15, 2020 11:54

gnehapk reviewed Apr 15, 2020

View reviewed changes

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

afreen23 suggested changes Apr 16, 2020

View reviewed changes

bipuladh force-pushed the refactor-install branch from 8d8f6a7 to 25d8a1c Compare April 16, 2020 07:50

Refactored Node Selection Page for OCS Installation Flow

db82f7f

bipuladh force-pushed the refactor-install branch from 25d8a1c to db82f7f Compare April 16, 2020 08:00

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

bipuladh requested review from afreen23 and gnehapk April 16, 2020 09:34

afreen23 mentioned this pull request Apr 16, 2020

Bug 1811003: Make storage class dropdown required #5071

Merged

openshift-merge-robot merged commit 748cf07 into openshift:master Apr 16, 2020

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

		// intentionally keeping the taint logic as its required in 4.3 and will be handled with checkbox selection
		// promises.push(...makeTaintNodesRequest(selectedData));

	return !_.isEmpty(node.spec.taints);
	return !_.isEmpty(node.spec?.taints);

-        {(isIndependent === false || mode === MODES.CONVERGED) && (
-          <CreateOCSServiceForm match={match} />
-        )}
+        {(isIndependent === false || mode === MODES.CONVERGED) && clusterServiceVersion (
+          <CreateOCSServiceForm match={match} />
+        )}

-  const [selectedNodes, setSelectedNodes] = React.useState(null);
-  const [visibleRows, setVisibleRows] = React.useState(null);
+  const [selectedNodes, setSelectedNodes] = React.useState<NodeKind[]>([]);
+  const [visibleRows, setVisibleRows] = React.useState<NodeKind[]>([]);

	{`${pluralize(selectedNodes?.length \|\| 0, 'node', 'nodes')} selected`}
	{`${pluralize(selectedNodes.length, 'node')} selected`}

	<label className="control-label" htmlFor="ocs-service-stoargeclass">
	<label className="control-label" htmlFor="ocs-service-capacity">

	const taints: Taint[] = node.spec.taints \|\| [];
	const taints: Taint[] = node.spec?.taints \|\| [];

	if (JSON.stringify(filteredData) !== JSON.stringify(visibleRows)) {
	if (!_.isEqual(filteredData, visibleRows)) {

	const [selectedNodes, setSelectedNodes] = React.useState<NodeKind[]>(null);
	const [selectedNodes, setSelectedNodes] = React.useState<NodeKind[]>([]);

Refactored Node Selection Page for OCS Installation Flow #5017

Refactored Node Selection Page for OCS Installation Flow #5017

Uh oh!

Conversation

bipuladh commented Apr 13, 2020

Uh oh!

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

Uh oh!

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!

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!

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 15, 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

gnehapk commented Apr 13, 2020 •

edited

Loading