(feat): Setup create storage system wizard #9438

afreen23 · 2021-07-07T20:32:01Z

The PR covers the following:

Adds Backing Storage
implements a wizard with incrementally enabled steps
This step creates the Storage System for an external Provider other than RHCS on clicking Next
This step otherwise, allow choosing the backing storage for Storage Cluster. The Storage Cluster is hidden from UI but it is created internally by wizard flow. The creation of a storage cluster happens in the last step and is not covered in this PR.
https://issues.redhat.com/browse/ODFE-83
UXD: https://marvelapp.com/prototype/g1h86ga/screen/78847971

Testing Pr:
Use the following URL to see the Storage System wizard.

http://localhost:9000/k8s/ns/openshift-storage/operators.coreos.com~v1alpha1~ClusterServiceVersion/ocs-operator.v4.7.2/odf.openshift.io~v1alpha1~StorageSystem/~new

Screenshots:

afreen23 · 2021-07-08T05:37:17Z

/assign @vbnrh

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

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

vbnrh · 2021-07-08T09:16:20Z

frontend/packages/ceph-storage-plugin/src/odf-external-providers/types.ts

@@ -0,0 +1,10 @@
+export type ExternalProvider = {


this could be moved inside ceph-storage-plugin/src/types instead of a separate dir.
the dir odf-external-providers seems odd to be sitting at the root of the plugin

This file and folder is to allow external providers to add their components e.g flash system.
I am keeping it separate so that I can add more details to it and maintain separate components for the external system.

vbnrh · 2021-07-08T09:18:14Z

frontend/packages/ceph-storage-plugin/src/odf-external-providers/external-providers.ts

+import { ExternalProvider } from './types';
+import { RHCS, StorageClusterIdentifier } from '../constants/create-storage-system';
+
+export const ODF_EXTERNAL_PROVIDERS: ExternalProvider[] = [


Similarly, these constants could be moved to ceph-storage-plugin/src/constants/create-storage-system.ts. Doesn't seem like this needs a separate place.

@vbnrh we don't want to do that.
This file and folder is to allow external providers to add their components e.g flash system.
I am keeping it separate so that I can add more details to it and maintain separate components for the external system.

can we ditch the dir and keep the files like external-provider.ts, external-provider-types ? Should accomplish the same unless you're trying to add lot of more files in later PRs into the dir

Yeah we want to add support for each of providers to have their own folder and add their dedicated step.

frontend/packages/ceph-storage-plugin/src/models.ts

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

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

...rage-plugin/src/components/create-storage-system/create-storage-system-steps/create-steps.ts

vbnrh · 2021-07-08T10:30:40Z

...rage-plugin/src/components/create-storage-system/create-storage-system-steps/create-steps.ts

+        nonRhcsExternalProviderStep,
+        {
+          canJumpTo: count >= 3,
+          ...commonSteps.reviewAndCreate,


small confusion, if storagecluster is already there, we are just showing the step for creating storageclass and then moving to review and create ?

Is it just to create FlashSystem CR ?

Shouldn't the same be applicable to other cases if storagecluster exists or not ?

It is Storage system creation, if OCS Storage System is already there we do not want to create that one.
RHCS itself uses Storage Cluster - an OCS Storage System, hence its only for external vendors

vbnrh · 2021-07-08T10:35:17Z

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

+  currentStep: StepState;
+  backingStorage: {
+    type: BackingStorageType;
+    externalProvider: string;


Suggested change

externalProvider: string;

externalProvider?: string;

Optional right ?

It's the state of the component. it's set by the external storage dropdown.
By default one external Provider will always be set.

ok makes sense :)

Suggested change

externalProvider: string;

externalProvider: RHCS | IBM_FLASH;

Reduce the scope of values that can be assigned to external providers

Expect some of changes in another PR.
Otherwise it will increase the scope. the next PR will standardize contribution mechanism for plugins.

afreen23 · 2021-07-09T15:45:46Z

I have added an error handling HOC that can be used for each step:
In case of loading:

In case of error, will display -"An error has occurred: {errorMessage}"

cc @vbnrh @yuvalgalanti

vbnrh · 2021-07-12T09:46:22Z

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

+  currentStep: StepState;
+  backingStorage: {
+    type: BackingStorageType;
+    externalProvider: string;


ok makes sense :)

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

...rage-plugin/src/components/create-storage-system/create-storage-system-steps/create-steps.ts

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

frontend/packages/ceph-storage-plugin/src/odf-external-providers/external-providers.ts

vbnrh · 2021-07-12T14:41:40Z

frontend/packages/ceph-storage-plugin/src/odf-external-providers/external-providers.ts

+import { ExternalProvider } from './types';
+import { RHCS, StorageClusterIdentifier } from '../constants/create-storage-system';
+
+export const ODF_EXTERNAL_PROVIDERS: ExternalProvider[] = [


can we ditch the dir and keep the files like external-provider.ts, external-provider-types ? Should accomplish the same unless you're trying to add lot of more files in later PRs into the dir

vbnrh · 2021-07-12T15:03:04Z

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

+  currentStep: StepState;
+  backingStorage: {
+    type: BackingStorageType;
+    externalProvider: string;


Suggested change

externalProvider: string;

externalProvider: RHCS | IBM_FLASH;

Reduce the scope of values that can be assigned to external providers

vbnrh · 2021-07-12T15:34:06Z

I have added an error handling HOC that can be used for each step:
In case of loading:

In case of error, will display -"An error has occurred: {errorMessage}"

cc @vbnrh @yuvalgalanti

Looks good although i'd prefer if the spinner is centered vertically and horizontally on the page. It looks a bit above center mark. Please check if you have time otherwise we can move on :-)

afreen23 · 2021-07-13T06:43:38Z

Looks good although i'd prefer if the spinner is centered vertically and horizontally on the page. It looks a bit above center mark. Please check if you have time otherwise we can move on :-)

I also wanted that, but this is rendered inside the div rendered by PF wizard. In order to center this , we need to center the wizard body which will then mess with other flows where we we do not want to do that.
I can also do something like

className={(!ssLoaded || !!ssLoadError) && 'odf-create-storage-system-wizard-body'}

But the issue with that one is, I need to maintain error states for the different steps in the parent component.
I would not do this much effort for the loading UIs. Open to other suggestions.

vbnrh · 2021-07-13T09:07:32Z

I just tested the PR, Couple of issues i noticed

StorageClass Dropdown is way too narrow and all the items are squeezed up. We need to make it wider

Next button is wider than the back, cancel. We have to keep them the same size

An error is thrown when selecting No default StorageClass from dropdown

The dropdown items are not rendered with the correct design. The design looks off ?

Additional margin/padding is required in between the breadcrumbs, title, wizard and it should show "Openshift Container Storage" as link instead of the ocs version.

Reference to how it is rendered on BucketClass creation page

afreen23 · 2021-07-13T09:14:08Z

I just tested the PR, Couple of issues i noticed
1. StorageClass Dropdown is way too narrow and all the items are squeezed up. We need to make it wider

Thats the optimum width we are using since previous releases

1. Additional margin/padding is required in between the breadcrumbs, title, wizard and it should show "Openshift Container Storage" as link instead of the ocs version.

If you check the code , I have added this intentionally. Too much space on header is making wizard smaller.

The dropdown items are not rendered with the correct design. The design looks off ?

Patternfly css https://www.patternfly.org/v4/components/form-select

Additional margin/padding is required in between the breadcrumbs, title, wizard and it should show "Openshift Container Storage" as link instead of the ocs version.

It takes in the name of resource you passing the url. If you check the previous version of code, you can see.
I have hardcoded it to be OpenShift Data Foundation for now to be consistent with other views

Rest I have fixed @vbnrh

vbnrh · 2021-07-13T09:21:00Z

I just tested the PR, Couple of issues i noticed
1. StorageClass Dropdown is way too narrow and all the items are squeezed up. We need to make it wider
Thats the optimum width we are using since previous releases
1. Additional margin/padding is required in between the breadcrumbs, title, wizard and it should show "Openshift Container Storage" as link instead of the ocs version.
If you check the code , I have added this intentionally. Too much space on header is making wizard smaller.

Hmm, looks bit too narrow.. Screen has plenty of space i feel it should be stretched out
I feel the wider spacing makes it look cleaner but UX may know best. Also we need to have consistent design with the other wizards
Why is FormSelect needed when we could use a normal Dropdrown ? Are there any particular advantages ? As per design guidelines
A form select embeds browser native select lists into a form.
If your use case only calls for simple selects, you may opt to use a form select as an field inside a form.

I see we are only selecting a couple of static items but using a browser native list makes it inconsistent with dropdowns in the same page and other wizard pages .

@yuvalgalanti Thoughts ?

afreen23 · 2021-07-13T09:43:58Z

3. Why is FormSelect needed when we could use a normal Dropdrown ? Are there any particular advantages ? As per design guidelines
   `A form select embeds browser native select lists into a form.`
   `If your use case only calls for simple selects, you may opt to use a form select as an field inside a form. `
I see we are only selecting a couple of static items but using a browser native list makes it inconsistent with dropdowns in the same page and other wizard pages .

We should not be choosing components purely based on aesthetics. We should consider the components which are semantically applicable for a UI element. Its is actually a select list option hence makes sense to use it here. Dropdown just renders the <button> . Additionally , you can add CSS to any component and get the same effect.
I would like to change Storage class as well, its in my todos.

vbnrh · 2021-07-13T11:01:54Z

3. Why is FormSelect needed when we could use a normal Dropdrown ? Are there any particular advantages ? As per design guidelines
   `A form select embeds browser native select lists into a form.`
   `If your use case only calls for simple selects, you may opt to use a form select as an field inside a form. `
I see we are only selecting a couple of static items but using a browser native list makes it inconsistent with dropdowns in the same page and other wizard pages .
We should not be choosing components purely based on aesthetics. We should consider the components which are semantically applicable for a UI element. Its is actually a select list option hence makes sense to use it here. Dropdown just renders the <button> . Additionally , you can add CSS to any component and get the same effect.
I would like to change Storage class as well, its in my todos.

Do we have any other places where FormSelect is being used ? if so we can proceed with it otherwise we better change it or make it look like the above dropdown. As more external providers are added, the design will only look more glaringly different over time.

I agree with your notion of using the proper component but Openshift team has always valued design consistency over what is logical which is why we have always made sure of all the text, capitalisation, colours, looks.

afreen23 · 2021-07-13T11:25:40Z

Do we have any other places where FormSelect is being used ? if so we can proceed with it otherwise we better change it or make it look like the above dropdown. As more external providers are added, the design will only look more glaringly different over time.

I agree with your notion of using the proper component but Openshift team has always valued design consistency over what is logical which is why we have always made sure of all the text, capitalisation, colours, looks.

Yep! I can see it used over a couple of places.

.../create-storage-system/create-storage-system-steps/backing-storage-step/backing-storage.scss

vbnrh · 2021-07-13T12:05:55Z

/approve

/assign @bipuladh @cloudbehl for the lgtm

frontend/packages/ceph-storage-plugin/console-extensions.json

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

frontend/packages/ceph-storage-plugin/src/odf-external-providers/external-providers.ts

bipuladh · 2021-07-14T04:04:34Z

frontend/packages/ceph-storage-plugin/src/odf-external-providers/external-providers.ts

+  /* name is a unique identifier that will be used in naming StorageSystems and sub-StorageSystems 
+     e.g `odf-<name>-storage-system`.
+  */
+  id: ExternalProviderId;


Why do we need both id and kind?

For nicer names and ease in reference. Its only the reference point for a provider
kind I have kept but it can be dropped further since it can be constructed easily from apiVersion, apiGroup and kind.

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

bipuladh · 2021-07-14T04:16:23Z

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

+  const onStorageClassSelect = (sc: StorageClassResourceKind) =>
+    dispatch({
+      type: 'wizard/setStorageClass',
+      payload: { name: sc?.metadata?.name, provisioner: sc?.provisioner },


Suggested change

payload: { name: sc?.metadata?.name, provisioner: sc?.provisioner },

payload: { name: sc.metadata.name, provisioner: sc.provisioner },

These are always defined, right? If a user selects a storage class then the object is present.

Not really, there is an option for no default storage/ no stoarge class class which is an empty object.

I think in that case as well the sc.provisioner will be added after figuring out the default storage class from the backend.

I meant no storage class, updated!

Y'all can check @vbnrh 's comment, he posted the error as well.
#9438 (comment)

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

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

frontend/packages/ceph-storage-plugin/locales/en/ceph-storage-plugin.json

frontend/packages/ceph-storage-plugin/package.json

cloudbehl · 2021-07-14T06:02:02Z

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

+  const onStorageClassSelect = (sc: StorageClassResourceKind) =>
+    dispatch({
+      type: 'wizard/setStorageClass',
+      payload: { name: sc?.metadata?.name, provisioner: sc?.provisioner },


I think in that case as well the sc.provisioner will be added after figuring out the default storage class from the backend.

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

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

bipuladh · 2021-07-14T06:38:14Z

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

+      return validateBackingStorageStep(type, storageClass, externalProvider);
+    case StepsId.CreateLocalVolumeSet:
+    case StepsId.CapacityAndNodes:
+    case StepsId.ConnectionDetails:


Suggested change

case StepsId.ConnectionDetails:

case StepsId.ConnectionDetails:

How are providers supposed to add their validation logic here?

Its for RHCS. RHCS has a very different view compared to external vendors.

The external vendors only use create storage class step, that only we will be standardizing.

bipuladh · 2021-07-14T06:40:02Z

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

+
+  return (
+    <>
+      {showErrorAlert && (


Suggested change

{showErrorAlert && (

{!_.isEmpty(requestError) && (

We only need one state.

Or simply requestError && (

Check the code below:
actionClose={<AlertActionCloseButton onClose={() => setShowErrorAlert(false)} />}
The state is for close button on alert and also using it to show alert.

bipuladh · 2021-07-14T06:41:45Z

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

+} from '@patternfly/react-core';
+import './create-storage-system.scss';
+
+export const CreateStorageSystemHeader: React.FC<CreateStorageSystemHeaderProps> = ({ url, t }) => (


We should use PageHeading here.

PageHeading is a very heavy component , and also not certain whether its part of sdk or not.(when I was coding this)
Plus, as per the recent changes, it would be the case that we can drop this header when the new operand extension would come.

I suggest we use PageHeading Operand Extension is going to take time. Page heading is going to be part of the SDK. As dashboards can't exist without PageHeading. The other reason to use PageHeading and an important one is to maintain style consistency across Pages with headers.

PageHeading itself wont be required with the operand extension.

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

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

- Adds Backing Storage step - implements a wizard with [incrementally enabled steps](https://www.patternfly.org/v4/components/wizard/#incrementally-enabled-steps) - This step creates the Storage System for an external Provider other than RHCS on clicking Next. - This step otherwise, allow choosing the backing storage for Storage Cluster. The Storage Cluster is hidden from UI but it is created internally by wizard flow. The creation of storage cluster happens in the last step. - https://issues.redhat.com/browse/ODFE-83 Signed-off-by: Afreen Rahman <afrahman@redhat.com>

bipuladh

/lgtm

openshift-ci · 2021-07-14T08:55:01Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,bipuladh,cloudbehl]

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

yuvalgalanti · 2021-07-15T10:59:57Z

I just tested the PR, Couple of issues i noticed

StorageClass Dropdown is way too narrow and all the items are squeezed up. We need to make it wider

Next button is wider than the back, cancel. We have to keep them the same size

An error is thrown when selecting No default StorageClass from dropdown

The dropdown items are not rendered with the correct design. The design looks off ?

Additional margin/padding is required in between the breadcrumbs, title, wizard and it should show "Openshift Container Storage" as link instead of the ocs version.

Reference to how it is rendered on BucketClass creation page

point 1: you are totally right lets make this filed wider
point 4: you right doesn't look like the PF$ dropdown
point 5: the padding you added looks good. lets see that its the same padding we have in other places as well.

afreen23 · 2021-07-15T19:30:52Z

point 1: you are totally right lets make this filed wider

this is fixed

point 4: you right doesn't look like the PF$ dropdown

this is a pf form select -> https://www.patternfly.org/v4/components/form-select
We should check why its css is so different with PF folks and fix then.

point 5: the padding you added looks good. lets see that its the same padding we have in other places as well.

openshift-ci bot requested review from bipuladh and cloudbehl July 7, 2021 20:32

openshift-ci bot added component/ceph Related to ceph-storage-plugin kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 7, 2021

afreen23 force-pushed the feat-implement-wizard branch 4 times, most recently from 92b28f0 to 86c9c39 Compare July 7, 2021 22:56

openshift-ci bot assigned vbnrh Jul 8, 2021

bipuladh suggested changes Jul 8, 2021

View reviewed changes

frontend/packages/ceph-storage-plugin/src/plugin.ts Show resolved Hide resolved

vbnrh suggested changes Jul 8, 2021

View reviewed changes

afreen23 force-pushed the feat-implement-wizard branch from 86c9c39 to d674885 Compare July 9, 2021 15:35

vbnrh reviewed Jul 12, 2021

View reviewed changes

afreen23 force-pushed the feat-implement-wizard branch 2 times, most recently from 3f8e73d to d84001a Compare July 13, 2021 05:31

afreen23 force-pushed the feat-implement-wizard branch from d84001a to e1eaf7c Compare July 13, 2021 09:56

afreen23 requested review from vbnrh and bipuladh July 13, 2021 10:03

afreen23 force-pushed the feat-implement-wizard branch from e1eaf7c to 18298e1 Compare July 13, 2021 11:51

afreen23 commented Jul 13, 2021

View reviewed changes

.../create-storage-system/create-storage-system-steps/backing-storage-step/backing-storage.scss Show resolved Hide resolved

afreen23 force-pushed the feat-implement-wizard branch 2 times, most recently from 2d06224 to c772153 Compare July 13, 2021 13:57

bipuladh suggested changes Jul 14, 2021

View reviewed changes

afreen23 force-pushed the feat-implement-wizard branch 2 times, most recently from 0fce634 to 1a40cf9 Compare July 14, 2021 06:00

cloudbehl reviewed Jul 14, 2021

View reviewed changes

afreen23 force-pushed the feat-implement-wizard branch from 1a40cf9 to f070cf5 Compare July 14, 2021 06:29

bipuladh suggested changes Jul 14, 2021

View reviewed changes

afreen23 force-pushed the feat-implement-wizard branch from f070cf5 to bd20139 Compare July 14, 2021 07:00

afreen23 force-pushed the feat-implement-wizard branch from bd20139 to 9386f1f Compare July 14, 2021 07:03

bipuladh approved these changes Jul 14, 2021

View reviewed changes

openshift-ci bot assigned bipuladh Jul 14, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021

cloudbehl approved these changes Jul 14, 2021

View reviewed changes

openshift-merge-robot merged commit 6427450 into openshift:master Jul 14, 2021

spadgett added this to the v4.9 milestone Jul 14, 2021

	externalProvider: string;
	externalProvider: RHCS \| IBM_FLASH;

	payload: { name: sc?.metadata?.name, provisioner: sc?.provisioner },
	payload: { name: sc.metadata.name, provisioner: sc.provisioner },

	case StepsId.ConnectionDetails:
	case StepsId.ConnectionDetails:

(feat): Setup create storage system wizard #9438

(feat): Setup create storage system wizard #9438

Conversation

afreen23 commented Jul 7, 2021 • edited

afreen23 commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Jul 8, 2021 • edited

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

afreen23 Jul 8, 2021 • edited

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

afreen23 commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbnrh commented Jul 12, 2021

afreen23 commented Jul 13, 2021 • edited

vbnrh commented Jul 13, 2021

afreen23 commented Jul 13, 2021 • edited

vbnrh commented Jul 13, 2021 • edited

afreen23 commented Jul 13, 2021 • edited

vbnrh commented Jul 13, 2021

afreen23 commented Jul 13, 2021

vbnrh commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Jul 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Jul 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Jul 14, 2021 • edited

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

afreen23 Jul 14, 2021 • edited

Choose a reason for hiding this comment

bipuladh Jul 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 14, 2021

yuvalgalanti commented Jul 15, 2021

afreen23 commented Jul 15, 2021 • edited

afreen23 commented Jul 7, 2021 •

edited

afreen23 Jul 8, 2021 •

edited

afreen23 Jul 8, 2021 •

edited

afreen23 commented Jul 13, 2021 •

edited

afreen23 commented Jul 13, 2021 •

edited

vbnrh commented Jul 13, 2021 •

edited

afreen23 commented Jul 13, 2021 •

edited

afreen23 Jul 14, 2021 •

edited

afreen23 Jul 14, 2021 •

edited

afreen23 Jul 14, 2021 •

edited

afreen23 Jul 14, 2021 •

edited

bipuladh Jul 14, 2021 •

edited

afreen23 commented Jul 15, 2021 •

edited