Kubevirt: add basic actions to VM list #1761

atiratree · 2019-06-19T14:39:38Z

depends on:

openshift-ci-robot · 2019-06-19T14:39:58Z

Hi @suomiy. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

spadgett

I've only reviewed the actions commit, but here are my comments.

spadgett · 2019-06-19T17:24:27Z

frontend/packages/kubevirt-plugin/src/components/vms/menu-actions.tsx

+  );
+};
+
+const menuActionStart = (kind, vm, actionArgs) => {


You'll want to add RBAC checks to these actions. For example,

https://github.com/openshift/console/blob/master/frontend/public/components/utils/kebab.tsx#L64-L78

spadgett · 2019-06-19T17:25:31Z

frontend/packages/kubevirt-plugin/src/components/modals/start-stop-vm-modal.tsx

+import { getName, getNamespace } from '@console/shared';
+import { VirtualMachineModel } from '../../models';
+
+class StartStopVmModal extends PromiseComponent<StartStopVmModalProps, PromiseComponentState> {


Prefer functional components for new components

spadgett · 2019-06-19T17:25:52Z

frontend/packages/kubevirt-plugin/src/components/modals/start-stop-vm-modal.tsx

+        <ModalTitle>{action} Virtual Machine</ModalTitle>
+        <ModalBody>
+          Are you sure you want to {action.toLowerCase()} <strong>{getName(vm)}</strong>
+          <span>


Unnecessary span?

spadgett · 2019-06-19T17:27:03Z

frontend/packages/kubevirt-plugin/src/components/modals/start-stop-vm-modal.tsx

+  vm: V1VirtualMachine;
+  start: boolean;
+  cancel: (e?: Event) => void;
+  close: (e?: Event) => void;


I don't see us passing the event to these callbacks in the implementation. We should take it out if we're not using it.

spadgett · 2019-06-19T17:28:02Z

frontend/packages/kubevirt-plugin/src/selectors/vm/index.ts

+
+import { V1VirtualMachine } from 'kubevirt-typescript-api/esm';
+
+export const isVmRunning = (value: V1VirtualMachine) => get(value, (v) => v.spec.running, false);


isVMRunning etc

atiratree · 2019-06-20T15:38:47Z

addressed all the comments

jtomasek · 2019-06-25T12:51:04Z

frontend/packages/kubevirt-plugin/src/components/vms/menu-actions.tsx

+import { asAccessReview, Kebab } from '@console/internal/components/utils';
+import { isVMRunning } from '../../selectors/vm';
+import { startStopVmModal } from '../modals/start-stop-vm-modal';
+import { K8sKind, PodKind } from '../../../../../public/module/k8s';


Suggested change

import { K8sKind, PodKind } from '../../../../../public/module/k8s';

import { K8sKind, PodKind } from '@console/internal/module/k8s';

Thanks, fixed.

jtomasek · 2019-06-25T12:51:42Z

frontend/packages/kubevirt-plugin/src/components/vms/vm.tsx

 } from '../../models';

+import { VMKind } from '../../types';
+import { menuActions } from './menu-actions';
+import { PodKind } from '../../../../../public/module/k8s';


Suggested change

import { PodKind } from '../../../../../public/module/k8s';

import { PodKind } from '@console/internal/module/k8s';

jelkosz · 2019-06-26T07:09:34Z

/ok-to-test

frontend/packages/kubevirt-plugin/src/components/vms/menu-actions.tsx

atiratree · 2019-06-26T12:25:18Z

fixed

atiratree · 2019-06-26T14:14:51Z

added types and new PR dependency

atiratree · 2019-06-26T16:10:02Z

/retest

atiratree · 2019-06-27T11:50:08Z

/retest

rawagner · 2019-06-27T13:18:30Z

/lgtm

openshift-ci-robot · 2019-06-27T13:18:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~frontend/packages/kubevirt-plugin/OWNERS~~ [rawagner,suomiy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 19, 2019

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 19, 2019

openshift-ci-robot requested review from jcaianirh and vojtechszocs June 19, 2019 14:40

atiratree force-pushed the kubevirt.vm-actions branch 2 times, most recently from 7aaf32b to f0bad12 Compare June 19, 2019 16:40

spadgett reviewed Jun 19, 2019

View reviewed changes

spadgett added this to the v4.2 milestone Jun 19, 2019

atiratree force-pushed the kubevirt.vm-actions branch from f0bad12 to 18f90d3 Compare June 20, 2019 15:37

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 20, 2019

atiratree mentioned this pull request Jun 20, 2019

expose build AccessReview functionality #1779

Merged

atiratree force-pushed the kubevirt.vm-actions branch from 18f90d3 to 60d37f0 Compare June 21, 2019 09:23

jtomasek reviewed Jun 25, 2019

View reviewed changes

atiratree force-pushed the kubevirt.vm-actions branch from 60d37f0 to fbaf526 Compare June 25, 2019 16:38

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 26, 2019

atiratree force-pushed the kubevirt.vm-actions branch from fbaf526 to 341d43e Compare June 26, 2019 08:59

rawagner reviewed Jun 26, 2019

View reviewed changes

frontend/packages/kubevirt-plugin/src/components/vms/menu-actions.tsx Outdated Show resolved Hide resolved

atiratree force-pushed the kubevirt.vm-actions branch from 341d43e to 8010a29 Compare June 26, 2019 12:25

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2019

atiratree force-pushed the kubevirt.vm-actions branch from 8010a29 to ead6e8a Compare June 26, 2019 14:11

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2019

atiratree force-pushed the kubevirt.vm-actions branch from ead6e8a to 2ae37fe Compare June 26, 2019 14:12

atiratree mentioned this pull request Jun 26, 2019

fix KebabOption type #1829

Merged

atiratree mentioned this pull request Jun 26, 2019

kubevirt: Add Status column to VmList #1689

Merged

7 tasks

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2019

kubevirt: add basic actions to VM list

7e5f703

atiratree force-pushed the kubevirt.vm-actions branch from 2ae37fe to 7e5f703 Compare June 27, 2019 10:23

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 27, 2019

openshift-ci-robot assigned rawagner Jun 27, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2019

openshift-merge-robot merged commit 8deb1a5 into openshift:master Jun 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubevirt: add basic actions to VM list #1761

Kubevirt: add basic actions to VM list #1761

atiratree commented Jun 19, 2019 •

edited

Loading

openshift-ci-robot commented Jun 19, 2019

spadgett left a comment

spadgett Jun 19, 2019

spadgett Jun 19, 2019

spadgett Jun 19, 2019

spadgett Jun 19, 2019

spadgett Jun 19, 2019

atiratree commented Jun 20, 2019 •

edited

Loading

jtomasek Jun 25, 2019

atiratree Jun 25, 2019

jtomasek Jun 25, 2019

jelkosz commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 27, 2019

rawagner commented Jun 27, 2019

openshift-ci-robot commented Jun 27, 2019


		import { V1VirtualMachine } from 'kubevirt-typescript-api/esm';

		export const isVmRunning = (value: V1VirtualMachine) => get(value, (v) => v.spec.running, false);

	import { K8sKind, PodKind } from '../../../../../public/module/k8s';
	import { K8sKind, PodKind } from '@console/internal/module/k8s';

	import { PodKind } from '../../../../../public/module/k8s';
	import { PodKind } from '@console/internal/module/k8s';

Kubevirt: add basic actions to VM list #1761

Kubevirt: add basic actions to VM list #1761

Conversation

atiratree commented Jun 19, 2019 • edited Loading

openshift-ci-robot commented Jun 19, 2019

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelkosz commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 26, 2019

atiratree commented Jun 27, 2019

rawagner commented Jun 27, 2019

openshift-ci-robot commented Jun 27, 2019

atiratree commented Jun 19, 2019 •

edited

Loading

atiratree commented Jun 20, 2019 •

edited

Loading