-
Notifications
You must be signed in to change notification settings - Fork 606
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
Pool management - delete #8550
Pool management - delete #8550
Conversation
|
||
export type OcsStorageClassResource = StorageClassResourceKind & { | ||
parameters: { | ||
pool: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing storage kind dont have StorageClassResourceKind dont have a parameter, also parameter will change dynamically, i am maintaining our OCS specific StorageClassResourceKind
1d4d086
to
27dc27d
Compare
c690455
to
0e7cf8c
Compare
/retest |
1 similar comment
/retest |
"Finish": "Finish", | ||
"Delete Block Pool": "Delete Block Pool", | ||
"{state.poolStatus === POOL_PROGRESS.BOUNDED ? (\n <p>\n <strong>{state.poolName}</strong> cannot be deleted. When a pool is bounded to\n PVC it cannot be deleted. Please detach all the resources from storage class(es){' '}\n <strong>{scNames.join(', ')}</strong>.{' '}\n <Button variant=\"link\" isInline onClick={pvcLink}>\n Go to PVC list\n </Button>\n </p>\n ) : (\n <p>\n Deleting <strong>{state.poolName}</strong> will remove all the saved data of\n this pool. Are you sure want to delete?\n </p>\n )}": "{state.poolStatus === POOL_PROGRESS.BOUNDED ? (\n <p>\n <strong>{state.poolName}</strong> cannot be deleted. When a pool is bounded to\n PVC it cannot be deleted. Please detach all the resources from storage class(es){' '}\n <strong>{scNames.join(', ')}</strong>.{' '}\n <Button variant=\"link\" isInline onClick={pvcLink}>\n Go to PVC list\n </Button>\n </p>\n ) : (\n <p>\n Deleting <strong>{state.poolName}</strong> will remove all the saved data of\n this pool. Are you sure want to delete?\n </p>\n )}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably by mistake ?
<Trans t={t} ns="ceph-storage-plugin"> | ||
{state.poolStatus === POOL_PROGRESS.BOUNDED ? ( | ||
<p> | ||
<strong>{state.poolName}</strong> cannot be deleted. When a pool is bounded to | ||
PVC it cannot be deleted. Please detach all the resources from storage class(es){' '} | ||
<strong>{scNames.join(', ')}</strong>.{' '} | ||
<Button variant="link" isInline onClick={pvcLink}> | ||
Go to PVC list | ||
</Button> | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Trans t={t} ns="ceph-storage-plugin"> | |
{state.poolStatus === POOL_PROGRESS.BOUNDED ? ( | |
<p> | |
<strong>{state.poolName}</strong> cannot be deleted. When a pool is bounded to | |
PVC it cannot be deleted. Please detach all the resources from storage class(es){' '} | |
<strong>{scNames.join(', ')}</strong>.{' '} | |
<Button variant="link" isInline onClick={pvcLink}> | |
Go to PVC list | |
</Button> | |
</p> | |
{state.poolStatus === POOL_PROGRESS.BOUNDED ? ( | |
<Trans t={t} ns="ceph-storage-plugin"> | |
<p> | |
<strong>{{state.poolName}}</strong> cannot be deleted. When a pool is bounded to | |
PVC it cannot be deleted. Please detach all the resources from storage class(es){' '} | |
<strong>{{scNames.join(', ')}}</strong>.{' '} | |
<Button variant="link" isInline onClick={pvcLink}> | |
Go to PVC list | |
</Button> | |
</p> | |
</Trans> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
<p> | ||
Deleting <strong>{state.poolName}</strong> will remove all the saved data of | ||
this pool. Are you sure want to delete? | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<p> | |
Deleting <strong>{state.poolName}</strong> will remove all the saved data of | |
this pool. Are you sure want to delete? | |
</p> | |
<Trans t={t} ns="ceph-storage-plugin"> | |
<p> | |
Deleting <strong>{{state.poolName}}</strong> will remove all the saved data of | |
this pool. Are you sure want to delete? | |
</p> | |
</Trans> |
this pool. Are you sure want to delete? | ||
</p> | ||
)} | ||
</Trans> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</Trans> |
(scNameList: string[]) => { | ||
const pvcScNames = pvcResources?.items?.map((pvcConfig) => pvcConfig?.spec?.storageClassName); | ||
// intersection of scNames and pvcScNames | ||
const usedScNames = scNameList.filter((scName) => pvcScNames.includes(scName)); | ||
if (usedScNames.length) { | ||
dispatch({ type: BlockPoolActionType.SET_POOL_STATUS, payload: POOL_PROGRESS.BOUNDED }); | ||
setScNames(usedScNames); | ||
} | ||
}, | ||
[pvcResources], | ||
); | ||
|
||
// find storage class using pool | ||
React.useEffect(() => { | ||
if (scLoaded && pvcLoaded) | ||
checkPvcStatus( | ||
scResources?.items | ||
?.filter((scConfig) => state.poolName === scConfig?.parameters?.pool) | ||
.map((scConfig) => scConfig?.metadata?.name) || [], | ||
); | ||
}, [scResources, scLoaded, pvcResources, pvcLoaded, state.poolName, checkPvcStatus]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find a lot of these helpers in selectors/index.ts
. You can leverage those and you should always create pure functions is possible.
For instance -> getPVStorageClass
for
const pvcScNames = pvcResources?.items?.map((pvcConfig) => pvcConfig?.spec?.storageClassName);
And set the state setScNames
in useEffect
41c0ff9
to
36e3168
Compare
onClick: () => void; | ||
action: 'Create' | 'Save' | 'Delete'; | ||
onPoolCreation?: (name: string) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this footer is shared by create | delete | update module so no more extra option required
@afreen23 Changes done |
36e3168
to
7d4bfaf
Compare
export const getPvcNames = (pvcResources: ListKind<K8sResourceKind>): string[] => | ||
pvcResources?.items?.map((pvcConfig) => getStorageClassName(pvcConfig)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its giving you sc names and not pvc names.
No need to create a new utility just use it directly in the component as follows:
export const getPvcNames = (pvcResources: ListKind<K8sResourceKind>): string[] => | |
pvcResources?.items?.map((pvcConfig) => getStorageClassName(pvcConfig)); | |
const scNames = pvcResources?.items?.map(getStorageClassName); |
export const getSCNamesUsingPool = ( | ||
scData: ListKind<K8sResourceKind>, | ||
poolName: string, | ||
): string[] => | ||
scData.items?.reduce((scNames, sc) => { | ||
if (_.get(sc, 'parameters.pool') === poolName) scNames.push(sc.metadata?.name); | ||
return scNames; | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not put this here, if its not going to be used by components other than pool.
export const getSCNamesUsingPool = ( | |
scData: ListKind<K8sResourceKind>, | |
poolName: string, | |
): string[] => | |
scData.items?.reduce((scNames, sc) => { | |
if (_.get(sc, 'parameters.pool') === poolName) scNames.push(sc.metadata?.name); | |
return scNames; | |
}, []); | |
export const getSCNamesUsingPool = ( | |
scData: ListKind<K8sResourceKind>, | |
poolName: string, | |
): string[] => | |
scData.items?.reduce((scNames, sc) => { | |
if (sc?.parameters?.pool === poolName) scNames.push(sc.metadata?.name); | |
return scNames; | |
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! got the point
const pvcScNames: string[] = getPvcNames(pvcResources) || []; | ||
// intersection of scNames and pvcScNames | ||
const usedScNames = scNameList.filter((scName) => pvcScNames.includes(scName)); | ||
if (usedScNames.length) { | ||
dispatch({ type: BlockPoolActionType.SET_POOL_STATUS, payload: POOL_PROGRESS.BOUNDED }); | ||
setScNames(usedScNames); | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can move this logic to use effect below itself.
You dont need callback since pvcResources
is a dependency there as well. Hence not providing the expected optimization.
@@ -63,17 +63,20 @@ export const BlockPoolModalFooter = (props: BlockPoolModalFooterProps) => { | |||
</Button> | |||
<Button | |||
type="button" | |||
variant="primary" | |||
variant={action !== 'Delete' ? 'primary' : 'danger'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to create an enum of all button texts.
Import and use pf ButtonVariant
enum.
variant={action !== 'Delete' ? 'primary' : 'danger'} | |
variant={action !== ButtonText.Delete ? ButtonVariant.primary' : ButtonVariant.danger'} |
7d4bfaf
to
b6724c2
Compare
/retest |
@@ -29,8 +29,8 @@ export const getCephPVs = (pvsData: K8sResourceKind[] = []): K8sResourceKind[] = | |||
); | |||
}); | |||
|
|||
const getPVStorageClass = (pv: K8sResourceKind) => _.get(pv, 'spec.storageClassName'); | |||
const getStorageClassName = (pvc: K8sResourceKind) => | |||
export const getPVStorageClass = (pv: K8sResourceKind) => _.get(pv, 'spec.storageClassName'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const getPVStorageClass = (pv: K8sResourceKind) => _.get(pv, 'spec.storageClassName'); | |
const getPVStorageClass = (pv: K8sResourceKind) => _.get(pv, 'spec.storageClassName'); |
const pvcScNames: string[] = pvcResources.items?.map((pvcConfig) => | ||
getStorageClassName(pvcConfig), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const pvcScNames: string[] = pvcResources.items?.map((pvcConfig) => | |
getStorageClassName(pvcConfig), | |
); | |
const pvcScNames: string[] = pvcResources.items?.map(getStorageClassName); |
|
||
React.useEffect(() => { | ||
if (scLoaded && pvcLoaded && state.poolStatus !== POOL_PROGRESS.NOTALLOWED) { | ||
const scNameList: string[] = scResources.items?.reduce((scList, sc) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const scNameList: string[] = scResources.items?.reduce((scList, sc) => { | |
const poolScNames: string[] = scResources.items?.reduce((scList, sc) => { |
ddc7a5c
to
92c06ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make the go to pvc list the primary action instead pf close, also add cancel.
<p> | ||
<strong>{{ poolName }}</strong> cannot be deleted. When a pool is bounded to PVC | ||
it cannot be deleted. Please detach all the resources from storage class(es){' '} | ||
<strong>{{ scNames }}</strong>.{' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont you want to destructure all array elements separated by comma
and the last element by and
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i accidentally removed the change, let me check how to add and for last element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
export const commaSeparatedString = (text: string[], t: TFunction): string => | ||
[text.slice(0, -1).join(', '), text.slice(-1)[0]].join( | ||
text.length < 2 ? '' : ` ${t('ceph-storage-plugin~and ')}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i uded ` ${t('ceph-storage-plugin~and ')}` is working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For translation, its better to not put any spaces with strings. Also there is a chance of accidental miss while modifying.
Also explicit indication of spaces misses the chance of removal while making any changes.
text.length < 2 ? '' : ` ${t('ceph-storage-plugin~and ')}`, | |
text.length < 2 ? '' : ` ${t('ceph-storage-plugin~and')} `, |
45c211f
to
01856e4
Compare
], | ||
[POOL_PROGRESS.NOTREADY]: [ | ||
{ | ||
id: 'modal-finish-action', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few states have similar prop but i don't want to create a new function to pass the prop, I feel let it be separate, so we can do changes in future easily
state={state} | ||
dispatch={dispatch} | ||
onClick={onClick} | ||
redirectToPvcList={redirectToPvcList} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally your footer should be generic to take two values vs onClick, redirect, actionTextButtonMapping.
- onSubmit - for primary text
- onCancel - for secondary text
I dont think you need an extra enum -actionTextButtonMapping
type: ButtonType.submit, | ||
variant: ButtonVariant.primary, | ||
onClick: () => { | ||
redirectToPvcList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redirectToPvcList(); | |
history.push('/k8s/all-namespaces/persistentvolumeclaims'); |
handlePromise(k8sKill(CephBlockPoolModel, blockPoolConfig), () => close()); | ||
}; | ||
|
||
const redirectToPvcList = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the error you are getting ? I assume you can call it there too.
closeLabel: string; | ||
actionButtonTextMapping: ActionButtonTextMapping; | ||
confirmAction: () => void; | ||
redirectToPvcList?: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redirectToPvcList?: () => void; |
actionLabel, | ||
closeLabel, | ||
confirmAction, | ||
redirectToPvcList, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redirectToPvcList, |
cancel={cancel} | ||
close={close} | ||
actionLabel={t('ceph-storage-plugin~Save')} | ||
closeLabel={t('ceph-storage-plugin~Close')} | ||
actionButtonTextMapping={ActionButtonTextMapping.UPDATE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actionButtonTextMapping={ActionButtonTextMapping.UPDATE} | |
primaryAction={FooterPrimaryActions.UPDATE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
@@ -274,3 +276,9 @@ export type Payload = K8sResourceCommon & { | |||
[key: string]: any; | |||
}; | |||
}; | |||
|
|||
export type OcsStorageClassResource = StorageClassResourceKind & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export type OcsStorageClassResource = StorageClassResourceKind & { | |
export type OcsStorageClassKind = StorageClassResourceKind & { |
@@ -208,3 +208,9 @@ export const checkRequiredValues = ( | |||
volumeType: string, | |||
isPoolManagementSupported: boolean, | |||
): boolean => !poolName || !replicaSize || (isPoolManagementSupported && !volumeType); | |||
|
|||
export enum ActionButtonTextMapping { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export enum ActionButtonTextMapping { | |
export enum FooterPrimaryActions { |
</Button> | ||
</ActionGroup> | ||
) : ( | ||
const modalFooterButtonFactory: ModalFooterButtonFactory = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const modalFooterButtonFactory: ModalFooterButtonFactory = { | |
const footerButtonsFactory: FooterButtonFactory = { |
label: t('ceph-storage-plugin~Cancel'), | ||
type: ButtonType.button, | ||
variant: ButtonVariant.secondary, | ||
onClick: () => cancel(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onClick: () => cancel(), | |
onClick: cancel |
01856e4
to
4958eb9
Compare
import { ModalComponentProps } from '@console/internal/components/factory/modal'; | ||
import { history } from '@console/internal/components/utils/router'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works after import history from the correct place
4958eb9
to
7ddb6dd
Compare
cancel={props.cancel} | ||
close={props.close} | ||
actionLabel={t('ceph-storage-plugin~Create')} | ||
closeLabel={t('ceph-storage-plugin~Finish')} | ||
footerPrimaryAction={FooterPrimaryActions.CREATE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since its footer component's prop
footerPrimaryAction={FooterPrimaryActions.CREATE} | |
primaryAction={FooterPrimaryActions.CREATE} |
}, [scResources, scLoaded, pvcResources, pvcLoaded, state.poolStatus, poolName, t]); | ||
|
||
// Delete block pool | ||
const onClick = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const onClick = () => { | |
const deletePool = () => { |
kind?: string; | ||
blockPoolConfig?: StoragePoolKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why all props optional ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I cant find where kind
is used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind is optional as per extension design we have to receive both, but kind is optional
}; | ||
|
||
type ModalFooterButtonFactory = { | ||
[stauts: string]: ButtonProps[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[stauts: string]: ButtonProps[]; | |
[status: POOL_PROGRESS]: ButtonProps[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it should be status in POOL_PROGRESS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reason i used string is, i used default string
disable?: boolean; | ||
onClick: (e?: React.FormEvent<EventTarget>) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable?: boolean; | |
onClick: (e?: React.FormEvent<EventTarget>) => void; | |
onClick: (e?: React.FormEvent<EventTarget>) => void; | |
disable?: boolean; |
cancel, | ||
close, | ||
} = props; | ||
const { state, dispatch, onSubmit, footerPrimaryAction, cancel, close } = props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this footer be used for create block pool as well ?
Just to have one footer - BlockPoolFooter
rather than one for modals and one for create.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can but it needs some hack for alignment, l thought it will confuse others, anyway, let me think what I can do here
7ddb6dd
to
a7baaaf
Compare
}; | ||
|
||
type FooterButtonFactory = { | ||
[stauts in POOL_PROGRESS | 'default']?: ButtonProps[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[stauts in POOL_PROGRESS | 'default']?: ButtonProps[]; | |
[status in POOL_PROGRESS | 'default']?: ButtonProps[]; |
const actionContentAlign = (buttons: JSX.Element[]) => | ||
className.includes('actions--left') ? buttons.reverse() : buttons; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why reversing buttons order ?
cancel={onClose} | ||
onSubmit={createPool} | ||
primaryAction={FooterPrimaryActions.CREATE} | ||
className="pf-c-form pf-c-form__actions--left" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why pf-c-form
on button actions ?
a7baaaf
to
257b9a0
Compare
Signed-off-by: Gowtham Shanmugasundaram <gshanmug@redhat.com>
257b9a0
to
3814595
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[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:
Approvers can indicate their approval by writing |
/retest |
1 similar comment
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/test e2e-gcp-console |
/retest Please review the full test history for this PR and help us cut down flakes. |
Signed-off-by: Gowtham Shanmugasundaram gshanmug@redhat.com