Feature: Expand cluster modal #2348
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
Hi @a2batic. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
size/L
|
Please add a description of what this change will do and why you are adding it. Please reference a bug or issue if there is one. |
a2batic
force-pushed
the
expand-cluster
branch
2 times, most recently
from
945868b to
076efdd
Compare
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Please clean up the formatting in this file
There was a problem hiding this comment.
CSS declarations should be alphabetized throughout this file
There was a problem hiding this comment.
Nit:
| .form-group input{ | |
| .form-group input { |
There was a problem hiding this comment.
Why use a dropdown if there is only one unit?
There was a problem hiding this comment.
@julienlim should we keep dropdowns for TiB and PiB then?
There was a problem hiding this comment.
Doesn't this have a unit? Are we assuming a certain unit?
There was a problem hiding this comment.
Why store this in state if it never changes?
There was a problem hiding this comment.
Does required not default to false?
There was a problem hiding this comment.
Remember to remove this before we're ready to merge
There was a problem hiding this comment.
This at least needs to be a K8sResourceKindReference
There was a problem hiding this comment.
nit: prefer obj which is used frequently in console
|
@alecmerdler fyi |
There was a problem hiding this comment.
| './components/modals/add-capacity-modal/add-capacity-modal' /* webpackChunkName: "ceph-storage-data-resiliency-card" */ | |
| './components/modals/add-capacity-modal/add-capacity-modal' /* webpackChunkName: "ceph-storage-add-capacity-modal" */ |
cloudbehl
left a comment
cloudbehl
left a comment
There was a problem hiding this comment.
Need to be consistent with modal instead of model.
There was a problem hiding this comment.
| export type AddCapacityModelProps = { | |
| export type AddCapacityModalProps = { |
There was a problem hiding this comment.
| <form onSubmit={submit} className="modal-content add-capacity-model__class"> | |
| <form onSubmit={submit} className="modal-content add-capacity-modal__class"> |
There was a problem hiding this comment.
| <div className="add-capacity-model__padding"> | |
| <div className="add-capacity-modal__padding"> |
There was a problem hiding this comment.
| .add-capacity-model__padding{ | |
| .add-capacity-modal__padding{ |
There was a problem hiding this comment.
| .add-capacity-model__class{ | |
| .add-capacity-modal__class{ |
There was a problem hiding this comment.
| export const AddCapacityModal = withHandlePromise((props: AddCapacityModelProps) => { | |
| export const AddCapacityModal = withHandlePromise((props: AddCapacityModalProps) => { |
|
/assign @gnehapk |
|
/assign @rawagner |
a2batic
force-pushed
the
expand-cluster
branch
from
076efdd to
1695b57
Compare
There was a problem hiding this comment.
| const units = ['TiB', 'PiB']; | |
| const units = _.keys(TopConsumerResourceValue); |
There was a problem hiding this comment.
There was a problem hiding this comment.
| const handleStorageClass = (sc: any) => { | |
| const handleStorageClass = (sc: K8sResourceKind) => { |
There was a problem hiding this comment.
describedBy not being used.
There was a problem hiding this comment.
Move required at the end.
There was a problem hiding this comment.
Move required at the end.
|
@a2batic is the alignment for the |
There was a problem hiding this comment.
Nit - remove this empty line
There was a problem hiding this comment.
Are handlePromise, isProgress, errorMessage props being used?
There was a problem hiding this comment.
No, I will remove them from Props
.
|
@alecmerdler, Please review |
|
/assign @alecmerdler |
|
@rawagner, thanks for helping out on Expand workflow. Please review. |
...ckages/ceph-storage-plugin/src/components/modals/add-capacity-modal/_add-capacity-modal.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Why are we hiding help-block? Can we just remove it from the DOM if it's not needed?
There was a problem hiding this comment.
Please don't reformat the entire file. It makes it hard to know what has really changed.
There was a problem hiding this comment.
Will this path always exist? Otherwise, this could be a runtime error. Consider _.get
| const presentCapacity = ocsConfig.spec.storageDeviceSets[0].count; | |
| const presentCapacity = _.get(ocsConfig, 'spec.storageDeviceSets[0].count'); |
There was a problem hiding this comment.
ID would trypically be storagecluster
There was a problem hiding this comment.
Why are we patching all of /spec if we're only changing one property?
You should not need to clone... You can just patch the single value.
There was a problem hiding this comment.
Is this closing the dialog? It will look like the action completed to the user. We should be showing the error instead.
You should look at the error handling in other dialogs for examples.
There was a problem hiding this comment.
Are we sure this full path will always exist? Otherwise it's a runtime error.
I would feel much more comfortable with a _.get
There was a problem hiding this comment.
How does the user know what unit is used?
There was a problem hiding this comment.
Since there is only one unit, it looks like this:-
There was a problem hiding this comment.
That's a placeholder text with no value.... That's not the right styling. I'd recommend just adding the one unit for now. It might be better to disable the dropdown as well, but you can skip that for now if it's not possible with the current component.
There was a problem hiding this comment.
By default the drop drown is hidden: To to show drop-drown, did it this way:-
There was a problem hiding this comment.
Use modal-content--no-inner-scroll instead
There was a problem hiding this comment.
BEM discourages the use of descendant selectors like this.
There was a problem hiding this comment.
Nit:
| .form-group input{ | |
| .form-group input { |
There was a problem hiding this comment.
Ideally the value here is a PF4 var.
a2batic
force-pushed
the
expand-cluster
branch
from
0640848 to
c2572e9
Compare
There was a problem hiding this comment.
These styles do not properly follow BEM naming.
We should avoid the nesting as well. We can address in a follow on.
There was a problem hiding this comment.
This action is missing an accessReview
There was a problem hiding this comment.
@spadgett how should we pass object to accessReview?
There was a problem hiding this comment.
accessReview: asAccessReview(kind, obj, 'patch'),You'd need to add another extension property to ClusterServiceVersionAction to handle this.
a2batic
force-pushed
the
expand-cluster
branch
from
c2572e9 to
3b3a0c3
Compare
openshift-ci-robot
added
the
needs-rebase
a2batic
force-pushed
the
expand-cluster
branch
from
3b3a0c3 to
159c91b
Compare
openshift-ci-robot
removed
the
needs-rebase
a2batic
force-pushed
the
expand-cluster
branch
from
159c91b to
8dad5ca
Compare
|
@spadgett addressed comments and rebased, please review |
spadgett
left a comment
spadgett
left a comment
There was a problem hiding this comment.
The storage class dropdown logic looks wrong.
There was a problem hiding this comment.
I'm confused: are we taking the ceph provider out of the array? It looks like you're removing them.
There was a problem hiding this comment.
@spadgett, we have to show the provides which are not ceph based, so we are creating another array getFilteredStorageClassData with storageClasses other than ceph based.
There was a problem hiding this comment.
You're not using the return type? filter doesn't mutate the original array. It returns a new value.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter
There was a problem hiding this comment.
This is doing a substring match, so you could have false positives. Use === if you want to match strings exactly. Assuming you want to keep the ceph providers, use
const cephStorageClasses = storageClassData.filter((sc: K8sRseourceKind) => {
return cephStorageProvisioners.some((provisioner: string) => sc.provisioner === provisioner);
});
a2batic
force-pushed
the
expand-cluster
branch
from
8dad5ca to
83a07cc
Compare
|
@spadgett thanks for the review on the filter, fixed the logic as per your suggestions. Please review. |
|
/test e2e-aws-console-olm |
a2batic
force-pushed
the
expand-cluster
branch
from
83a07cc to
a8f3c5f
Compare
Fixes: https://jira.coreos.com/browse/RHSTOR-504 Signed-off-by: Kanika <[email protected]>
a2batic
force-pushed
the
expand-cluster
branch
from
a8f3c5f to
4243acb
Compare
spadgett
left a comment
spadgett
left a comment
There was a problem hiding this comment.
/approve
/lgtm
/hold cancel
Follow on items:
- Access reviews to disable the action for users who don't have authority
- Storage class dropdown refactor (add
filterprop instead of creating a new wrapper) - Update CSS to follow BEM naming and other modal styles
|
|
||
| const filteredSCData = scData.filter((sc: K8sResourceKind) => { | ||
| return cephStorageProvisioners.every( | ||
| (provisioner: string) => !_.get(sc, 'provisioner').includes(provisioner), |
There was a problem hiding this comment.
Are you sure you want includes? That is a substring match.
There was a problem hiding this comment.
@spadgett right now provisioners are coming in the form of NAMESPACE.cephfs.csi.ceph.com NAMESPACE.rbd.csi.ceph.com ceph.rook.io/block, where Namespace is not constant and can be more than one word, so used includes to check for substring that will be present for sure.
There was a problem hiding this comment.
OK, I see. The storage stuff is not in my wheelhouse :)
openshift-ci-robot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a2batic, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
14 participants
Please Merge after : #2359
Fixes: https://jira.coreos.com/browse/RHSTOR-504