-
Notifications
You must be signed in to change notification settings - Fork 605
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
Alert when stopping a vm with logged in users #5733
Alert when stopping a vm with logged in users #5733
Conversation
@pcbailey please review |
30b7ab7
to
176ba44
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.
Just a few minor comments for the moment.
}; | ||
|
||
return ( | ||
<form onSubmit={submit} className="modal-content"> | ||
<ModalTitle> | ||
<YellowExclamationTriangleIcon className="co-icon-space-r" /> Delete{' '} | ||
{isTemplate(vmLikeEntity) ? 'Virtual Machine Template' : entityModel.label}? | ||
{VirtualMachineModel.label}? |
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.
Now that the ternary operator is removed, I think this can be written more cleanly as a template literal.
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.
fixed
Are you sure you want to delete{' '} | ||
<strong className="co-break-word">{getName(vmLikeEntity)}</strong> in namespace{' '} | ||
<strong>{getNamespace(vmLikeEntity)}</strong> ? | ||
Are you sure you want to delete <strong className="co-break-word">{name}</strong> in |
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 looks much cleaner. =)
<form onSubmit={submit} className="modal-content"> | ||
<ModalTitle> | ||
<YellowExclamationTriangleIcon className="co-icon-space-r" /> Delete Virtual Machine | ||
Template ? |
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.
Should there be a space before all of the question marks here? I see several like this.
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.
fixed
import { VMIKind } from '../../../types'; | ||
import { VirtualMachineInstanceModel } from '../../../models'; | ||
|
||
const redirectFn = (vmi: VMIKind) => { |
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 feel like this should be made more general and shared among all the different modals.
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.
moved to a utils
file
bf65f18
to
667789b
Compare
frontend/packages/kubevirt-plugin/src/components/modals/menu-actions-modals/delete-vm-modal.tsx
Outdated
Show resolved
Hide resolved
...kages/kubevirt-plugin/src/components/modals/menu-actions-modals/delete-vm-template-modal.tsx
Outdated
Show resolved
Hide resolved
...kages/kubevirt-plugin/src/components/modals/menu-actions-modals/delete-vm-template-modal.tsx
Outdated
Show resolved
Hide resolved
deleteOwnedVolumeResources: deleteDisks, | ||
}); | ||
} | ||
const promise = deleteVM(vmUpToDate, { |
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.
+1 for splitting the functionality, makes it more readable
import { useUpToDateVMLikeEntity } from '../../../hooks/use-vm-like-entity'; | ||
import { deleteVMTemplate } from '../../../k8s/requests/vmtemplate/actions'; | ||
|
||
const redirectFn = (vmLikeEntity: TemplateKind) => { |
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.
naming
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.
How about something like redirectAfterDeletion
?
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.
renamed
} | ||
}; | ||
|
||
export const DeleteVMIModal = withHandlePromise((props: DeleteVMIProps) => { |
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.
Should we also ask for disk removal in standalone VMIs which do not belong to any VM resource?
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.
added :-)
|
||
const name = getName(vmi); | ||
const namespace = getNamespace(vmi); | ||
const alertHref = `/k8s/ns/${namespace}/virtualmachines/${name}/details#logged-in-users`; |
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 about standalone VMIs?
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 !!! wow, no I remember why I took it out to be a separate modal !
</> | ||
); | ||
} else if (userListLength > 1) { | ||
alertBody = ( |
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 feels like it should be parametrized
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.
Agreed. I think this can be done in one Button
component using pluralize
. I don't think it will hurt anything if we use a numeral 1 instead of "One".
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.
+2 :-) fixing
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.
fixed
c81de9f
to
b8ffdb6
Compare
import { getName, getNamespace } from '@console/shared/src/selectors/common'; | ||
import { VMILikeEntityKind } from 'packages/kubevirt-plugin/src/types/vmLike'; | ||
|
||
export const redirectAfterVMDeletion = (vmi: VMILikeEntityKind) => { |
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 we use this one for the template as well? And parametrize the /virtualization
part? I guess better name could be redirectToList
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.
yes, made it one method, much nicer now
<form onSubmit={submit} className="modal-content"> | ||
<ModalTitle> | ||
<YellowExclamationTriangleIcon className="co-icon-space-r" /> Delete Virtual Machine | ||
Instanse ? |
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.
Instanse ? | |
Instance ? |
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.
👍
b8ffdb6
to
20cc086
Compare
20cc086
to
0e80679
Compare
0e80679
to
96ba940
Compare
96ba940
to
b3e78e7
Compare
namespace <strong>{namespace}</strong>? | ||
{numOfAllResources > 0 && ( | ||
<p> | ||
The following resources will be deleted along with this virtual machine instance. |
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.
If I understand this correctly, this message is wrong. It looks like this is displaying the number of disks and giving a single checkbox to indicate consent to delete them or not. This message implies that there should be a list containing each of the individual resources with their own checkbox, where the user can pick which resources to delete and which to leave.
import { getName, getNamespace } from '@console/shared/src/selectors/common'; | ||
import { VMGenericLikeEntityKind } from 'packages/kubevirt-plugin/src/types/vmLike'; | ||
|
||
export const redirectToList = (vmi: VMGenericLikeEntityKind, tab?: 'templates' | '' | null) => { |
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.
Yay! =)
<Button variant="link" isInline onClick={onLinkClick}> | ||
{pluralize(userListLength, 'User')} | ||
</Button>{' '} | ||
currently logged in to this VM. Proceeding with this operation may cause logged in users data |
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 would change the last sentence to "Proceeding with this operation may cause logged in users to lose data."
I'm going to approve this. We can fix these two messages in a follow-up. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pcbailey, yaacov 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 |
Alert when stoping a vm with logged in users
Desing: http://openshift.github.io/openshift-origin-design/designs/virtualization/4.x/expose-guest-data/expose%20guest%20data.html#details-tab
Screenshot: