-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
mgr/dashboard: subvolume rm with snapshots #53182
mgr/dashboard: subvolume rm with snapshots #53182
Conversation
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.
Thanks @Pegonzal! left a couple of reviews
...oard/frontend/src/app/ceph/cephfs/cephfs-subvolume-list/cephfs-subvolume-list.component.html
Outdated
Show resolved
Hide resolved
...hboard/frontend/src/app/ceph/cephfs/cephfs-subvolume-list/cephfs-subvolume-list.component.ts
Outdated
Show resolved
Hide resolved
...oard/frontend/src/app/ceph/cephfs/cephfs-subvolume-list/cephfs-subvolume-list.component.html
Outdated
Show resolved
Hide resolved
...oard/frontend/src/app/ceph/cephfs/cephfs-subvolume-list/cephfs-subvolume-list.component.html
Outdated
Show resolved
Hide resolved
4b327f7
to
ebdc4cb
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.
Thanks @Pegonzal, this looks almost perfect.
I noticed some weird behaviour with the critical-confirmation modal. If you click the Cancel
button for the first time in the modal it's giving an error in the console log. I checked and it looks like something I introduced a while ago. The fix would be easy, would you mind adding that here?
this line needs to be
(backActionEvent)="backAction ? callBackAction() : hideModal()"
also, another thing is that it says Unknown Task in the notification message
Unrelated bug (could be dealt in a separate PR): The path is /unknown/.../unknown
when removed, but ideally it should be empty.
...hboard/frontend/src/app/ceph/cephfs/cephfs-subvolume-list/cephfs-subvolume-list.component.ts
Outdated
Show resolved
Hide resolved
ebdc4cb
to
06257dc
Compare
Thanks for the reviews @nizamial09, I fixed everything you mentioned on this PR itself, let me know what you think |
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.
Good work. Lgtm! Thank you @Pegonzal
Looks like the e2e is failing and we'll need to adapt it. Earlier there were only one checkbox so using the class This line needs to be modified. Probably to something like |
Fixes: https://tracker.ceph.com/issues/62452 Signed-off-by: Pedro Gonzalez Gomez <pegonzal@redhat.com>
06257dc
to
453fbcb
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
Fixes: https://tracker.ceph.com/issues/62452
Add option to delete subvolume that have snapshots
screen-capture.25.mp4
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows