Skip to content
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

Adding the ability to start in pause mode #772

Merged

Conversation

DanaOrr
Copy link
Contributor

@DanaOrr DanaOrr commented Jul 18, 2022

Signed-off-by: Dana Orr dorr@redhat.com

📝 Description

Adding "Start in Pause mode" item to the Details tab.

🎥 Demo

image

image

image

@openshift-ci openshift-ci bot requested review from glekner and tnisan July 18, 2022 13:31
@DanaOrr DanaOrr force-pushed the adding-start-in-pause-mode branch from 79b99b4 to 0ebad86 Compare July 18, 2022 14:01
Copy link
Member

@avivtur avivtur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if we change the text from mode to status here and here
it's something we need to verify with UX
other than that nice work 👍

@DanaOrr DanaOrr force-pushed the adding-start-in-pause-mode branch from 0ebad86 to a5efc43 Compare July 20, 2022 10:06
{
hasPendingChange: startStrategyChanged,
tabLabel: VirtualMachineDetailsTabLabel.Details,
label: t('Start in Pause Mode'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change to label: t('Start in pause mode')

<FormGroup
fieldId="start-pause-mode"
helperText={t(
'Applying the Start/Pause mode to this Virtual Machine will cause it to partially reboot and pause.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change to Applying the start/pause mode to this VirtualMachine will cause it to partially reboot and pause.

id="start-pause-mode"
isChecked={checked}
onChange={setChecked}
label={t('Start this VirtualMachine in Pause mode')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change to Start this VirtualMachine in pause mode

@@ -257,6 +258,24 @@ const VirtualMachineDetailsLeftGrid: React.FC<VirtualMachineDetailsLeftGridProps
}
data-test-id={`${vm?.metadata?.name}-boot-method`}
/>
<VirtualMachineDescriptionItem
descriptionData={vm?.spec?.template?.spec?.startStrategy ? t('ON') : t('OFF')}
descriptionHeader={t('Start in Pause Mode')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change to Start in pause mode

isOpen={isOpen}
onClose={onClose}
onSubmit={onSubmit}
headerText={t('Start in Pause Mode')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change to Start in pause mode

const updatedVirtualMachine = React.useMemo(() => {
const updatedVM = produce<V1VirtualMachine>(vm, (vmDraft: V1VirtualMachine) => {
ensurePath(vmDraft, ['spec.template.spec']);
vmDraft.spec.template.spec.startStrategy = checked ? 'Paused' : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use printableVMStatus.Paused instead of hard-coded 'Paused'

isOpen={isOpen}
onClose={onClose}
onSubmit={onSubmit}
headerText={t('Start in Pause Mode')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change to Start in pause mode

const startStrategyChanged = getChangedStartStrategy(
vm,
vmi,
!!vm?.spec?.template?.spec?.startStrategy,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be extracted from vm, no need to send another arg :)

tabLabel: VirtualMachineDetailsTabLabel.Details,
label: t('Start in Pause Mode'),
handleAction: () => {
history.push(getTabURL(vm, VirtualMachineDetailsTab.Details));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed?

@DanaOrr DanaOrr force-pushed the adding-start-in-pause-mode branch from a5efc43 to 688ef1c Compare July 21, 2022 08:30
Signed-off-by: Dana Orr <dorr@redhat.com>
@DanaOrr DanaOrr force-pushed the adding-start-in-pause-mode branch from 688ef1c to 01ee9bc Compare July 21, 2022 08:36
@metalice
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label Jul 21, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DanaOrr, metalice

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved This issue is something we want to fix label Jul 21, 2022
@openshift-merge-robot openshift-merge-robot merged commit b9095ea into kubevirt-ui:main Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This issue is something we want to fix lgtm Passed code review, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants