-
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
CNV-5024: Inform for pending changes in VM details view #5633
CNV-5024: Inform for pending changes in VM details view #5633
Conversation
ed6b730
to
95ab4a2
Compare
frontend/packages/kubevirt-plugin/src/components/vms/vm-details.tsx
Outdated
Show resolved
Hide resolved
(key) => | ||
vmConfChanges[key] && ( | ||
<ListItem key={key}> | ||
<Button onClick={() => openModal(key)} isInline variant="link"> |
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 is it better to open the modals this way? Don't we want them to go to the original modal location and just show the summary of what changed here?
frontend/packages/kubevirt-plugin/src/components/vms/vm-details.scss
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/modals/vm-flavor-modal/vm-flavor-modal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/modals/boot-order-modal/boot-order-modal.tsx
Outdated
Show resolved
Hide resolved
@@ -243,6 +273,15 @@ export const VMSchedulingList: React.FC<VMSchedulingListProps> = ({ | |||
const nodeSelector = vmiLikeWrapper?.getNodeSelector(); | |||
const tolerationsWrapperCount = (vmiLikeWrapper?.getTolerations() || []).length; | |||
const affinityWrapperCount = getRowsDataFromAffinity(vmiLikeWrapper?.getAffinity())?.length; | |||
const vmConfChanges = detectNextRunChanges(vm, vmi); | |||
|
|||
React.useEffect(() => { |
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.
again
|
||
export class VMIWrapper extends K8sResourceWrapper<VMIKind, VMIWrapper> implements VMILikeMethods { | ||
constructor(vmi?: VMIKind | VMIWrapper | any, copy = false) { | ||
constructor(vmi: VMIKind | VMIWrapper | any, copy = false) { |
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?
it should be optional in case you want to create empty VMI
super(VirtualMachineInstanceModel, vmi, copy); | ||
} | ||
|
||
getVMIObj = () => this.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.
we already have a function for that
}) => { | ||
const isVM = kindObj === VirtualMachineModel; | ||
const vmiLike = isVM ? vm : vmi; | ||
const vmiLike = !isVMRunningOrExpectedRunning(vm) ? vm : vmi; |
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 will display wrong information about VM's, once you change for example flavor
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 feature is about pending changes for the next run.
If a vm is running you want to show the current configuration while the pending changes you can see it the modal - this way you can see the difference between what you have now and what will you have after restart.
It make much more sense to me.
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 does it make more sense to show the VMI configuration? and not VM configuration?
I think it is a bit weird from user perspective.
- you click on flavor
- change flavor in modal
- the flavor you clicked on is not changed
- you will start asking if I clicked cancel or save?
- also the user might expect the VM details to respect the VM yaml
- you have to click modal to see the real values
- if you are interested in the VMI view - you can find a VMI page
Anyway this change will also cause inconsistencies.
- why is it okay to show VMI cdroms and not disks?
- why do we show VMI info of scheduling part, but keeping the os for VM? plus you cannot believe the overview of scheduling modal will have something different inside
IMO, this will cause that the users will be unsure what is real info on VM's page and what is not.
Can we just show a link to VMI and describe which fields are not included in VMI configuration?
getVolumePersistentVolumeClaimName(getVMIVolumes(vmi).find((vol) => vol.name === diskName)); | ||
|
||
export const getVMIBootableDisks = (vmi: VMIKind, defaultValue: V1Disk[] = []): V1Disk[] => | ||
getVMIDisks(vmi, defaultValue).filter((disk) => !Object.keys(disk).includes('serial')); |
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 are we ignoring disks with serial?
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.
disks with serial are environment variable disks (cm, secret, sa) which aren't bootable
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.
Other types of disks can't have serial filled?
95ab4a2
to
2480e2d
Compare
@irosenzw can you please move this to WIP since this still has some work to be done? |
2480e2d
to
9d97653
Compare
9d97653
to
9846aa0
Compare
dad5725
to
1c0a0db
Compare
/retest |
adb6c4f
to
b84f199
Compare
b84f199
to
f76ca1d
Compare
@@ -10,3 +14,14 @@ export const getVMIApiPath = (vmi: VMIKind) => | |||
`${VirtualMachineInstanceModel.apiVersion}/namespaces/${getNamespace(vmi)}/${ | |||
VirtualMachineInstanceModel.plural | |||
}/${getName(vmi)}`; | |||
|
|||
export const getVMIPVCSourceByDisk = (vmi: VMIKind, diskName: 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.
where are these 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.
done.
@@ -150,14 +150,23 @@ export const getCDRoms = (vm: VMILikeEntityKind) => | |||
? getDisks(vm as VMKind).filter((device) => !!device.cdrom) | |||
: getVMIDisks(vm as VMIKind).filter((device) => !!device.cdrom); | |||
|
|||
export const getContainerImageByDisk = (vm: VMKind, name: string) => | |||
getVolumeContainerImage(getVolumes(vm).find((vol) => name === vol.name)); | |||
export const getContainerImageByDisk = (vm: VMILikeEntityKind, name: 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.
are these changes required? Can we keep the old version?
return transformDevices(getVMIDisks(vmi), getVMIInterfaces(vmi)); | ||
}; | ||
|
||
const getVMIBootableDisks = (vmi: VMIKind, defaultValue: V1Disk[] = []): V1Disk[] => { |
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 refactor the original function and pass just disks and volumes to it instead?
@@ -0,0 +1,181 @@ | |||
import { VMWrapper } from '../../k8s/wrapper/vm/vm-wrapper'; |
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.
let's use console conventions for naming the files
this.getVolumes().find((vol) => vol.name === volName); | ||
|
||
getVolumesOfDisks = (disks: V1Disk[]): V1Volume[] => { | ||
const diskNames = (disks || this.getDisks()).map((disk) => disk?.name); |
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 usage always passes disks so no need for || this.getDisks()
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 create a set here instead?
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 would a set help?
vmLikeEntity?: FirehoseResult<VMLikeEntityKind>; | ||
}; | ||
|
||
const getPendingChanges = ( |
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 already is here! Why do we have to send it from lower components?
): PendingChanges => ({ | ||
[IsPendingChange.flavor]: { | ||
isPendingChange: isFlavorChanged(vmWrapper, vmiWrapper), | ||
func: () => vmFlavorModal({ vmLike: vm, blocking: true }), |
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.
please let's use a better name than func
obj: vmLikeEntity, | ||
customData: { updatePendingChanges }, | ||
}) => { | ||
const resources = [ |
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 should not be necessary
const [isLocked, setIsLocked] = useSafetyFirst(false); | ||
const withProgress = wrapWithProgress(setIsLocked); | ||
|
||
const loadedVMIs = vmis && getLoadedData(vmis); | ||
const vmi = loadedVMIs && loadedVMIs.length > 0 && loadedVMIs[0]; |
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.
huh, how does this even work?
const inProgress = props.inProgress || !loaded; | ||
const vm = asVM(vmLike); | ||
const underlyingTemplate = getLoadedData(template); | ||
const loadedVMIs = getLoadedData(vmis); | ||
const vmi = loadedVMIs && loadedVMIs.length > 0 && loadedVMIs[0]; |
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.
same. ??
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 we just load one VMI entity?
fc1be5b
to
9feaa51
Compare
9feaa51
to
5265505
Compare
7895f7e
to
ff2bc6c
Compare
const vmEnvDisks = vm.getDisks().filter((dsk) => vmEnvDiskVolumeNames.includes(dsk.name)); | ||
const vmiEnvDisks = vmi.getDisks().filter((dsk) => vmiEnvDiskVolumeNames.includes(dsk.name)); | ||
|
||
return isDisksChanged(vm, vmi, vmEnvDisks, vmiEnvDisks); |
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 passing of just the names like we talked about?
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.
that's not going to be enough cause for the envDisks the names aren't enough - I have to get the disks anyway.
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 know, but it would be a simpler interface and you can use disks or volumes indiscriminately
@@ -10,3 +13,11 @@ export const getVMIApiPath = (vmi: VMIKind) => | |||
`${VirtualMachineInstanceModel.apiVersion}/namespaces/${getNamespace(vmi)}/${ | |||
VirtualMachineInstanceModel.plural | |||
}/${getName(vmi)}`; | |||
|
|||
export const getVMIDataVolumeNameByDisk = (vmi: VMIKind, diskName: string): 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.
these two are not used anywhere right?
return transformDevices(getVMIDisks(vmi), getVMIInterfaces(vmi)); | ||
}; | ||
|
||
const getVMIBootableDisks = (vmi: VMIKind): V1Disk[] => { |
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 one is not used
return getBootableDisks(null, getVMIVolumes(vmi), getVMIDisks(vmi)); | ||
}; | ||
|
||
export const getVMIBootableDevices = (vmi: VMIKind): BootableDeviceType[] => { |
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.
not used
if (isPaused) { | ||
if (arePendingChanges) { | ||
icon = YellowExclamationTriangleIcon; | ||
} else if (isPaused) { | ||
icon = PausedIcon; | ||
} else if (status === VMStatusEnum.RUNNING) { | ||
icon = SyncAltIcon; |
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 should go under this if, this component should not show pending changes for other states and prefer errors etc
const thisResource = this.asResource(); | ||
|
||
if (!omitRuntimeData) { | ||
return _.isEqual(thisResource, volume); |
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 use this.data
instead
@@ -143,4 +144,33 @@ export class VolumeWrapper extends ObjectWithTypePropertyWrapper< | |||
return null; | |||
} | |||
} | |||
|
|||
areVolumesEqual = (volume: V1Volume, omitRuntimeData?: boolean) => { |
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.
areVolumesEqual = (volume: V1Volume, omitRuntimeData?: boolean) => { | |
areVolumesEqual = (otherVolume: V1Volume, omitRuntimeData?: boolean) => { |
or just other?
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.
could we do the same for disks?
|
||
return !vmVolumes.every((vol) => { | ||
const vmDisk = vmDiskLookup[vol.name]; | ||
const diskWrapper = new DiskWrapper(vmDisk); |
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 on one line
const vmiDevices = getVMIDevices(vmi.asResource()); | ||
|
||
let isDevsEqual = true; | ||
vmDevices.forEach((bootableDevice, index) => { |
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 would be easier to use every
or some
here
|
||
const thisResource = this.asResource(); | ||
|
||
if (!omitRuntimeData) { |
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 move this to the beginning
@@ -41,6 +43,14 @@ export class VMIWrapper extends K8sResourceWrapper<VMIKind, VMIWrapper> implemen | |||
|
|||
getVolumes = (defaultValue = []) => getVMIVolumes(this.data, defaultValue); | |||
|
|||
getVolumeByName = (volName: string): V1Volume => |
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.
not used
@@ -60,20 +60,30 @@ export class VMWrapper extends K8sResourceWrapper<VMKind, VMWrapper> implements | |||
|
|||
getVolumes = (defaultValue = []) => getVolumes(this.data, defaultValue); | |||
|
|||
getVolumeByName = (volName: string): V1Volume => |
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.
not used
getVolumesByType = (volType: VolumeType): V1Volume[] => | ||
this.getVolumes().filter((vol) => new VolumeWrapper(vol).getType() === volType); | ||
|
||
getConfigMaps = (): V1Volume[] => this.getVolumesByType(VolumeType.CONFIG_MAP); |
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
vm={vm} | ||
vmi={vmi} | ||
vmStatusBundle={vmStatusBundle} | ||
arePendingChanges={arePendingChanges && isVMRunningOrExpectedRunning(vm)} |
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.
note: this can cause to be evaluated to true even when VM is not in a running state
vmStatusBundle={vmStatusBundle} | ||
arePendingChanges={arePendingChanges && isVMRunningOrExpectedRunning(vm)} | ||
/> | ||
{isVMRunningOrExpectedRunning(vm) && arePendingChanges && <div>Pending changes</div>} |
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 make one boolean out of this?
import { VirtualMachineModel, VirtualMachineInstanceModel } from '../../models'; | ||
|
||
import './pending-changes-warning.scss'; | ||
import { PENDING_CHANGES_WARNING_MESSAGE } from '../../strings/vm/status'; |
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.
dem orders
.kv-warning-changed-attrs { | ||
display: flex; | ||
flex-direction: row; | ||
} |
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.
newline
kind: VirtualMachineInstanceModel.kind, | ||
namespace: getNamespace(vmLike), | ||
prop: 'vmis', | ||
isList: true, |
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 just load the one entity we need?
{ | ||
kind: VirtualMachineInstanceModel.kind, | ||
namespace: getNamespace(asVM(vmLikeEntity)), | ||
prop: '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.
plus here
} = props; | ||
const vm = asVM(vmLikeEntity); | ||
|
||
const [storageClassConfigMap, isStorageClassConfigMapLoaded] = useStorageClassConfigMap(); | ||
const inProgress = _inProgress || !isStorageClassConfigMapLoaded; | ||
const loadedVMIs = getLoadedData(vmis); | ||
const vmi = loadedVMIs && loadedVMIs.length > 0 && loadedVMIs[0]; |
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.
..
ff2bc6c
to
4c2c611
Compare
@@ -10,3 +11,6 @@ export const getVMIApiPath = (vmi: VMIKind) => | |||
`${VirtualMachineInstanceModel.apiVersion}/namespaces/${getNamespace(vmi)}/${ | |||
VirtualMachineInstanceModel.plural | |||
}/${getName(vmi)}`; | |||
|
|||
export const getVMIURLSourceByDisk = (dv: K8sResourceKind) => |
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.
not used
import { history } from '@console/internal/components/utils/router'; | ||
import { VMKind, VMIKind } from '../types'; | ||
|
||
const getTabURL = (vm: VMKind, tabName: VMTabURLEnum) => |
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 have this func in ./url
?
@@ -71,4 +71,31 @@ export class DiskWrapper extends ObjectWithTypePropertyWrapper< | |||
}; | |||
} | |||
} | |||
|
|||
isDisksEqual = (otherDisk: V1Disk, omitRuntimeData?: boolean): boolean => { |
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 name suggests there are multiple disks. Can we use either isDiskEqual
or isEqual
@@ -143,4 +144,31 @@ export class VolumeWrapper extends ObjectWithTypePropertyWrapper< | |||
return null; | |||
} | |||
} | |||
|
|||
isVolumesEqual = (otherVolume: V1Volume, omitRuntimeData?: boolean) => { |
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.
plus here
.getServiceAccounts() | ||
.find((vol) => vol.serviceAccount.serviceAccountName === sourceName); | ||
|
||
const getVolumeBySource = (sourceName: string, vmWrapper: VMWrapper) => |
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.
ping
|
||
export const hasPendingChanges = (vm: VMKind, vmi: VMIKind): boolean => { | ||
const pendingChanges = !!vmi && getPendingChanges(new VMWrapper(vm), new VMIWrapper(vmi)); | ||
return Object.keys(pendingChanges).reduce( |
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 will crash if pendingChanges get null
}).result, | ||
) | ||
} | ||
isDisabled={isLocked || isVMRunningOrExpectedRunning(asVM(vmLikeEntity))} | ||
isDisabled={isLocked} |
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.
we should allow editing for the nics/disks as well if we allow creating them
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.
@suomiy we cannot allow editing / deleting disks and nics at the moment, since it could result unexpected behavior.
For example: adding a disk -> add it to the boot order -> delete the disk. result an invalid boot order.
I believe there are other edge cases like this.
IMO, we should have this functionality, but it needs to be checked and tested and not added at the last moment. I would like to avoid making the PR unstable.
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 should be the same and we should be detecting all relevant changes since it goes all through yaml. Anyway I don't mind adding this later
key={`pcViewDetailsBtn-${getName(vm)}`} | ||
variant={ButtonVariant.plain} | ||
onMouseUp={() => | ||
history.push(`/ns/${getNamespace(vm)}/virtualmachines/${getName(vm)}/details`) |
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.
getTabUrl?
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.
also there are some other places in the code where this could be used
4c2c611
to
12df37f
Compare
return false; | ||
} | ||
|
||
switch (volType) { |
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 the exact logic of getVolumeReferencedObject
. So you can do volWrapper.getVolumeReferencedObject()?.name === sourceName
instead
and isEnvType is ensuring you are having the correct type
const volWrapper = new VolumeWrapper(vol); | ||
const volType = volWrapper.getType(); | ||
|
||
if (!volType.isEnvType()) { |
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 (!volType.isEnvType()) { | |
if (!volType?.isEnvType()) { |
NPE
12df37f
to
c864ca4
Compare
<PendingChangesAlert isWarning> | ||
{PENDING_CHANGES_WARNING_MESSAGE} | ||
<List className="kv-warning--pf-c-list"> | ||
{Object.keys(pendingChangesByTab).map( |
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.
{Object.keys(pendingChangesByTab).map( | |
{Object.keys(getPendingChangesByTab(pendingChanges)).map( |
IMO better to move here, since we do not need the variable before early 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'm using pendingChangesByTab
later too.
moving the variable creation after the early 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.
+1
const arePendingChanges = hasPendingChanges(vm, vmi, pendingChanges); | ||
const pendingChangesByTab = getPendingChangesByTab(pendingChanges); | ||
|
||
if (Object.keys(pendingChanges).length === 0 || !arePendingChanges) { |
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 use lodash _.isEmpty here since it also covers null and is easier to read
Present a warning when the user make changes while the VM is running Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com> CNV-5024# Please enter the commit message for your changes. Lines starting
c864ca4
to
ad24404
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irosenzw, rawagner, suomiy 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 |
Add alerts when the user make changes in the VM details view
while the VM is running informing to restart the VM for
the changes to apply.
Additionally, the changes aren't reflected immediately on the screen
while the VM is running, like they used to be, but change after the
VM restarts. This way the screen represents the current configuration
rather than the 'future' configuration.
Signed-off-by: Ido Rosenzwig irosenzw@redhat.com
CNV-5024# Please enter the commit message for your changes. Lines starting