Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extension for customized components for provisioner #5842

Merged
merged 1 commit into from Jul 16, 2020

Conversation

a2batic
Copy link
Contributor

@a2batic a2batic commented Jun 29, 2020

Helps to add customized components for provisoners in storage class creation form.

  • Adds extension getStorageClassProvisioner to fetch provisioners defined in different operators
  • Provides a way to add custom components for Provisioners
  • Introduces an attribute value for parameters, which help to take default values of provisioner's parameter

Screenshot from 2020-07-01 02-59-27

Thanks @rawagner
cc @rawagner @cloudbehl @bipuladh

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/ceph Related to ceph-storage-plugin labels Jun 29, 2020
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/sdk Related to console-plugin-sdk labels Jun 29, 2020
@a2batic a2batic marked this pull request as draft June 30, 2020 06:00
@a2batic a2batic force-pushed the poc-sc-plugin branch 2 times, most recently from 14452bf to 909bbeb Compare June 30, 2020 21:27
@a2batic a2batic changed the title [WIP] Extension for customized components for proviosner Extension for customized components for proviosner Jun 30, 2020
@a2batic a2batic marked this pull request as ready for review June 30, 2020 21:29
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2020
@a2batic
Copy link
Contributor Author

a2batic commented Jun 30, 2020

/assign @spadgett @bipuladh

Comment on lines 84 to 86
type: 'StorageClass/Proviosner',
properties: {
getStorageClassProvisoner,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type: 'StorageClass/Proviosner',
properties: {
getStorageClassProvisoner,
type: 'StorageClass/Provisioner',
properties: {
getStorageClassProvisioner,

Comment on lines 3 to 16
namespace ExtensionProperties {
export interface StorageClassProviosner {
getStorageClassProvisoner: () => any;
}
}

export interface StorageClassProviosner
extends Extension<ExtensionProperties.StorageClassProviosner> {
type: 'StorageClass/Proviosner';
}

export function isStorageClassProvisioner(e: Extension): e is StorageClassProviosner {
return e.type === 'StorageClass/Proviosner';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specific to OCS hence we can keep it in ceph package. If required in future we can move to shared.
We already have one extensions folder under /src

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we contribute type definitions to SDK from plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afreen23 imo, we shouldnt contribute type definations to sdk from plugins if they are to be used in public component.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this PR. Sorry for late response.
Many plugins have their extensions in their own package. We have one for ceph too already (which is stale now).
I think ceph csi is the consumer for this extension hence moving to ceph package makes sense to me.

import { Extension } from '@console/plugin-sdk';
interface DashboardsExtensionProperties {
/** Feature flags which are required for this extension to be effective. */
required?: string | string[];
/** Feature flags which are disallowed for this extension to be effective. */
disallowed?: string | string[];
}
namespace ExtensionProperties {
export interface DashboardsStorageCapacityDropdownItem extends DashboardsExtensionProperties {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afreen23 Im not sure I follow. The current place is ok. ceph-storage-plugin/src/extensions/*.ts is for other plugins that want to extend ceph-storage-plugin. In this case we are extending core so the extension definition should live in console-plugin-sdk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afreen23 the plugin is generic, it can be used both by csi and non-csi provisioners. It can help put custom components to any provisioner. imo, Its better to keep it in SDK for its better usage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afreen23 Im not sure I follow. The current place is ok. ceph-storage-plugin/src/extensions/*.ts is for other plugins that want to extend ceph-storage-plugin. In this case we are extending core so the extension definition should live in console-plugin-sdk.

Agree, seems like I was suggesting to derail from current patterns. My bad !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afreen23 the plugin is generic, it can be used both by csi and non-csi provisioners. It can help put custom components to any provisioner. imo, Its better to keep it in SDK for its better usage.

Sure @a2batic 👍

kind: referenceForModel(CephBlockPoolModel),
namespaced: false,
isList: true,
prop: 'pool',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
prop: 'pool',
prop: 'pools',

Comment on lines 22 to 23
public getStorageClassProvisioner() {
return this.extensions.filter(isStorageClassProvisioner);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You dont need registry when you use the useExtension hook.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
registry is deprecated so we should not add any new code here

Comment on lines 71 to 99
getExtensionsStorageClassProvisioners = (provionerType: string) => {
const extensionCSIProvisoners = {};
_.forEach(plugins.registry.getStorageClassProvisioner(), (operator: any) => {
if (operator) {
_.forEach(operator.properties.getStorageClassProvisoner(), (params, type) => {
extensionCSIProvisoners[type] = params;
});
}
});
return extensionCSIProvisoners[provionerType];
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the useExtension hook.
const scProvisioners = useExtension<StorageClassProvisioner>(isStorageClassProvisioner);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooks cannot be used inside class components.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use withExtensions HOC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This is still a class based component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand withExtensions will work here, but the aim is to do minimal changes in public component to add the extension.

}
}

export interface StorageClassProviosner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits ! fix everywhere

Suggested change
export interface StorageClassProviosner
export interface StorageClassProvisioner

type: 'StorageClass/Proviosner';
}

export function isStorageClassProvisioner(e: Extension): e is StorageClassProviosner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use arrow functions

Comment on lines 71 to 99
getExtensionsStorageClassProvisioners = (provionerType: string) => {
const extensionCSIProvisoners = {};
_.forEach(plugins.registry.getStorageClassProvisioner(), (operator: any) => {
if (operator) {
_.forEach(operator.properties.getStorageClassProvisoner(), (params, type) => {
extensionCSIProvisoners[type] = params;
});
}
});
return extensionCSIProvisoners[provionerType];
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use withExtensions HOC.

import { CaretDownIcon } from '@patternfly/react-icons';

const PoolResourceComponent: React.FC<PoolParams> = ({ onParamChange }) => {
const [data, loaded, loadError] = useK8sWatchResource<K8sResourceKind[]>(cephBlockPoolResource);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const [data, loaded, loadError] = useK8sWatchResource<K8sResourceKind[]>(cephBlockPoolResource);
const [poolData, poolDataLoaded, poolDataLoadError] = useK8sWatchResource<K8sResourceKind[]>(cephBlockPoolResource);


const PoolResourceComponent: React.FC<PoolParams> = ({ onParamChange }) => {
const [data, loaded, loadError] = useK8sWatchResource<K8sResourceKind[]>(cephBlockPoolResource);
const [isOpen, setOpen] = React.useState<boolean>(false);

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's my background as a Java programmer, but I have a strong preference for isOpen/setOpen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works! 👍

When I was reading setOpen it looked to me like its the setter for some other variable not for isOpen.

onParamChange(e.currentTarget.id);
};

const poolDropdownItems = _.map(data, (pool, key) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const poolDropdownItems = _.map(data, (pool, key) => (
const poolDropdownItems = data.map((pool, key) => (

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would need to be data?.map if not using lodash

@@ -0,0 +1,174 @@
import * as React from 'react';
import * as _ from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import * as _ from 'lodash';

@@ -1,6 +1,7 @@
import * as _ from 'lodash';

import { ActivePlugin, Extension, isKebabActions } from './typings';
import { isStorageClassProvisioner } from './typings/storage-class-params';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spell check

Comment on lines 1 to 5
import { StorageClassFormProvisoners } from '../components/ocs-storage-class-form';

export const getStorageClassProvisoner = () => {
return StorageClassFormProvisoners;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doing this? why not just import StorageClassFormProvisoners where ever required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@@ -64,105 +65,29 @@ export class StorageClassForm_ extends React.Component<

storageTypes = {};

// Fetch Storage type provisoners from different operators
// For CSI - provionerType: 'csi'
// For Defaults - provionerType: 'other'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// For Defaults - provionerType: 'other'
// For Defaults - provionerType: 'others'

// Fetch Storage type provisoners from different operators
// For CSI - provionerType: 'csi'
// For Defaults - provionerType: 'other'
getExtensionsStorageClassProvisioners = (provionerType: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getExtensionsStorageClassProvisioners = (provionerType: string) => {
getExtensionsStorageClassProvisioners = (provionerType = 'others') => {

Comment on lines 576 to 610
addDefaultParams = () => {
const defaultParams = this.state.newStorageClass.type
? this.storageTypes[this.state.newStorageClass.type].parameters
: {};
const hiddenParmas = {};
_.each(defaultParams, (values, param) => {
if (values.visible && !values.visible() && values.value) {
hiddenParmas[param] = values;
}
});
const value = { ...this.state.newStorageClass.parameters, ...hiddenParmas };
const newParams = {
...this.state.newStorageClass,
parameters: value,
};

return newParams;
};

Copy link
Contributor

@cloudbehl cloudbehl Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Need to relook into this.

const PoolResourceComponent: React.FC<PoolParams> = ({ onParamChange }) => {
const [data, loaded, loadError] = useK8sWatchResource<K8sResourceKind[]>(cephBlockPoolResource);
const [isOpen, setOpen] = React.useState<boolean>(false);
const [poolName, setPoolName] = React.useState('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const [poolName, setPoolName] = React.useState('');
const [poolName, setPoolName] = React.useState<string>('');

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicit from ''. It would only be necessary when used null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since type has been added to isOpen too, either we remove that too or add it here.

const [poolName, setPoolName] = React.useState('');

const handleDropdownChange = (e) => {
setPoolName(e.currentTarget.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setPoolName(e.currentTarget.id);
const id = e.currentTarget.id;
setPoolName(id);
onParamChange(id);

const [isOpen, setOpen] = React.useState<boolean>(false);
const [poolName, setPoolName] = React.useState('');

const handleDropdownChange = (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const handleDropdownChange = (e) => {
const handleDropdownChange = (e: React.FormEvent<HTMLInputElement>) => {

onParamChange(e.currentTarget.id);
};

const poolDropdownItems = _.map(data, (pool, key) => (
Copy link
Contributor

@gnehapk gnehapk Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poolDropdownItems will be reloaded each time the component's state changes. Shouldn't this be inside React.useEffect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnehapk I have changed the resource type from FirehoseResource to WatchK8sResource. It will restrict the number of calls for Pool list.

title="Select Pool"
className="dropdown dropdown--full-width"
toggle={
<DropdownToggle id="toggle-id" onToggle={onToggle} iconComponent={CaretDownIcon}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can directly call setOpen(!isOpen); here instead of separate function.

setOpen(!isOpen);
};
const onSelect = React.useCallback(() => {
setOpen(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this value be always false here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the dropdown should close everytime we select a value.

}
isOpen={isOpen}
dropdownItems={poolDropdownItems}
onSelect={onSelect}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can directly call setOpen here

};

type PoolParams = {
onParamChange: Function;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
onParamChange: Function;
onParamChange: (id: string) => void;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes much of a difference.

Copy link
Contributor

@gnehapk gnehapk Jul 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does here as (id: string) or (id: string, name:string) for eg. will both fit for Function and that's not we follow across other views.

@@ -842,6 +791,11 @@ export class StorageClassForm_ extends React.Component<
return null;
}

if (parameter.Component) {
const Component = parameter.Component;
Copy link
Contributor

@bipuladh bipuladh Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const Component = parameter.Component;
const { Component } = parameter;

const onToggle = () => {
setOpen(!isOpen);
};
const onSelect = React.useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be a callback. You can make it a regular function.

@a2batic a2batic changed the title Extension for customized components for proviosner Extension for customized components for provisioner Jul 14, 2020
@@ -13,3 +13,4 @@ export const OCS_INDEPENDENT_CR_NAME = 'ocs-external-storagecluster';
export const OCS_CONVERGED_CR_NAME = 'ocs-storagecluster';
export const NO_PROVISIONER = 'kubernetes.io/no-provisioner';
export const OCS_SUPPORT_ANNOTATION = 'features.ocs.openshift.io/enabled';
export const OCS_NAMESPACE = 'openshift-storage';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at line 5.

value?: string;
visible?: () => boolean;
required?: boolean;
Component?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Component?: any;
component?: React.Component;

// For CSI - provionerType: 'csi'
// For Defaults - provionerType: 'others'
getExtensionsStorageClassProvisioners = (provionerType = 'others') => {
const extensionCSIProvisoners = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const extensionCSIProvisoners = {};
const extensionCSIProvisioners = {};

Comment on lines 73 to 75
// Fetch Storage type provisoners from different operators
// For CSI - provionerType: 'csi'
// For Defaults - provionerType: 'others'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Fetch Storage type provisoners from different operators
// For CSI - provionerType: 'csi'
// For Defaults - provionerType: 'others'
// Fetch Storage type provisoners from different operators
// For CSI - provionerType: 'csi'
// For Defaults - provionerType: 'others'
enum Provisioner {
CSI = 'csi',
OTHERS = 'others'
}

// Fetch Storage type provisoners from different operators
// For CSI - provionerType: 'csi'
// For Defaults - provionerType: 'others'
getExtensionsStorageClassProvisioners = (provionerType = 'others') => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getExtensionsStorageClassProvisioners = (provionerType = 'others') => {
getExtensionsStorageClassProvisioners = (provisionerType = Provisioner.OTHERS) => {

Comment on lines 583 to 585
const defaultParams = this.state.newStorageClass.type
? this.storageTypes[this.state.newStorageClass.type].parameters
: {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const defaultParams = this.state.newStorageClass.type
? this.storageTypes[this.state.newStorageClass.type].parameters
: {};
const defaultParams = this.storageTypes?.[this.state?.newStorageClass?.type]?.parameters ?? {};

k8sCreate(StorageClassModel, data)
.then(() => {
this.setState({ loading: false });
history.push('/k8s/cluster/storageclasses');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a new thing. But isn't the default side effect of creation is that we move to the details page of the created resource? @spadgett

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's true, We need to move to the details page if the resource is created.

I saw mockups in UX call of CNV & storage where there will be a success page and use can go to multiple pages from there but that not implemented right now.

Comment on lines 780 to 782
handleComponentParams = (value: string, key: string) => {
this.setParameterHandler(key, value, false);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
handleComponentParams = (value: string, key: string) => {
this.setParameterHandler(key, value, false);
};
handleComponentParams = (value: string, key: string) => this.setParameterHandler(key, value, false);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is only used in one place. We could also use it directly. but it's up to you.

Copy link
Contributor

@cloudbehl cloudbehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good mostly.

onToggle={() => setOpen(!isOpen)}
toggleIndicator={CaretDownIcon}
>
{poolName || 'Select a Pool'}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not select a pool by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloudbehl just trying to follow the pattern. In storageClass form, we dont have any dropdowns with default selection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the ease of use. I'm not sure about pattern.

@a2batic
Copy link
Contributor Author

a2batic commented Jul 15, 2020

/test e2e-gcp-console

@@ -0,0 +1,69 @@
import * as React from 'react';
import { Dropdown } from '@patternfly/react-core/dist/js/components/Dropdown/Dropdown';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the long path

Suggested change
import { Dropdown } from '@patternfly/react-core/dist/js/components/Dropdown/Dropdown';
import { Dropdown, DropdownItem, DropdownToggle } from '@patternfly/react-core';


const poolDropdownItems = poolData?.map((pool, key) => (
<DropdownItem
key={key}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
key={key}
key={pool.metadata.uid}


return (
<>
{poolDataLoaded && !poolDataLoadError && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if the fetch fails we show nothing ? Dont you want to show some kind of error message ?

Copy link
Contributor Author

@a2batic a2batic Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, added error message for this.


namespace ExtensionProperties {
export interface StorageClassProvisioner {
getStorageClassProvisioner: () => ExtensionSCProvisionerProp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to have this as function and not just

Suggested change
getStorageClassProvisioner: () => ExtensionSCProvisionerProp;
storageClassProvisioner: ExtensionSCProvisionerProp;

?

// For Defaults - provisionerType: 'others'
getExtensionsStorageClassProvisioners = (provisionerType = Provisioner.OTHERS) => {
const extensionCSIProvisioners = {};
const externsionItems = this.props.params.map((item) => item.properties);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could simplify the map and following _.forEach into one reduce

value?: string;
visible?: () => boolean;
required?: boolean;
Component?: React.ComponentType<any>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be any but

Suggested change
Component?: React.ComponentType<any>;
Component?: React.ComponentType<ProvisionerProps>;
export type ProvisionerProps = {
  onParamChange: (id: string) => void;
};

You can also reuse the ProvisionerProps in PoolResourceComponent

this.props.params,
(res, value) => {
const obj = value.properties.getStorageClassProvisioner || {};
if (obj) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you still need to return res

Suggested change
if (obj) {
if (obj) {
const key = provisionerType;
const keyValue = obj[provisionerType];
res[key] = keyValue;
}
return res;

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2020
@a2batic
Copy link
Contributor Author

a2batic commented Jul 15, 2020

/test e2e-gcp-console

@a2batic a2batic requested a review from rawagner July 15, 2020 16:06
@a2batic
Copy link
Contributor Author

a2batic commented Jul 16, 2020

/test e2e-gcp-console

Helps to add customized components for provisioners in storage class creation form.

- Adds extension getStorageClassProvisioner to fetch provisioners defined in different operators
- Provides a way to add custom components for Provisioners
- Introduces an attribute value for parameters, which help to take default values of provisioner's parameter

Signed-off-by: Kanika <kmurarka@redhat.com>
@a2batic
Copy link
Contributor Author

a2batic commented Jul 16, 2020

@rawagner addressed comments, please review.

@rawagner
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, gnehapk, rawagner

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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2020
@openshift-merge-robot openshift-merge-robot merged commit f5683ed into openshift:master Jul 16, 2020
@spadgett spadgett added this to the v4.6 milestone Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/sdk Related to console-plugin-sdk lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants