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
KMS flow for storage class creation #7330
KMS flow for storage class creation #7330
Conversation
Skipping CI for Draft Pull Request. |
c3b7f1b
to
ba390fc
Compare
d2cd237
to
5cc7407
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.
nit:
// const updateCsiConfigObj: ConfigMapKind = { | ||
// apiVersion: ConfigMapModel.apiVersion, | ||
// kind: ConfigMapModel.kind, | ||
// data: { | ||
// [kms.name.value]: JSON.stringify(csiConfigData), | ||
// }, | ||
// metadata: { | ||
// name: KMSConfigMapCSIName, | ||
// namespace: CEPH_STORAGE_NAMESPACE, | ||
// }, | ||
// }; |
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 you remove this or put a comment why this is required?
<Button | ||
variant="plain" | ||
onClick={() => { | ||
if (editKMS) { |
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.
move to a function
91088c2
to
dc41d5a
Compare
dc41d5a
to
c014a0f
Compare
c014a0f
to
99a34bc
Compare
const updateKMS = async () => { | ||
const allServiceNames = csiConfigMap ? Object.keys(csiConfigMap?.data) : []; | ||
if ( | ||
(allServiceNames.length && allServiceNames.indexOf(state.kms.name.value) === -1) || | ||
!csiConfigMap | ||
) { | ||
try { | ||
const promises: Promise<K8sResourceKind>[] = createKmsResources( | ||
state.kms, | ||
editKMS, | ||
csiConfigMap?.data, | ||
); | ||
await Promise.all(promises).then(() => setEditKMS(false)); | ||
setErrorMessage(''); | ||
} catch (error) { | ||
setErrorMessage(error.message); | ||
} finally { | ||
setInProgress(false); | ||
} | ||
} else { | ||
setErrorMessage(`KMS service ${state.kms.name.value} already exist`); | ||
} | ||
}; |
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 you also need to setInProgress
initially ?
And make it false later.
const StorageClassEncryptionLabel: React.FC = () => ( | ||
<div className="ocs-storageClass-encryption__pv-title"> | ||
<span className="ocs-storageClass-encryption__pv-title--padding">Enable Encryption</span> | ||
<TechPreviewBadge /> | ||
</div> | ||
); | ||
|
||
const EncryptionCardLoading: React.FC = () => ( | ||
<Grid className="skeleton-box"> | ||
<GridItem span={12} className="skeleton-activity" /> | ||
<GridItem span={12} className="skeleton-activity" /> | ||
<GridItem span={12} className="skeleton-activity" /> | ||
<GridItem span={12} className="skeleton-activity" /> | ||
</Grid> | ||
); |
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.
Components dont need to be inside existing component.
These should be moved out
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [csiConfigMap]); | ||
|
||
return <></>; |
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 cannot understand the use of this component, if this returns nothing.
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 because we need to send a value without rendering anything....since plugin accepts a component only, its like this.
providerNamespace: kmsData.VAULT_NAMESPACE, | ||
hasHandled: true, | ||
}; | ||
setEncryptionDispatch(SCActionType.SET_KMS_ENCRYPTION, '', dispatch, kmsObj); |
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 should be dispatching on click of an action ideally .
This will set the state whenever map is not empty -> which does not seems like ideal condition
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.
The reason for doing this is to set initial state of component with respect to value of configMap, so as config map loads, we have to set the state.
React.useEffect(() => { | ||
onParamChange(checked.toString()); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [checked]); |
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 can be just inside if
99a34bc
to
38a985f
Compare
Signed-off-by: Kanika Murarka <kmurarka@redhat.com>
38a985f
to
081ff72
Compare
/test backend |
/test analyze |
1 similar comment
/test analyze |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a2batic, cloudbehl 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 |
/test backend |
/test e2e-gcp-console |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
Merge after: #7153