-
Notifications
You must be signed in to change notification settings - Fork 667
Kubevirt: add basic actions to VM list #1761
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
Kubevirt: add basic actions to VM list #1761
Conversation
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
7aaf32b to
f0bad12
Compare
spadgett
left a comment
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've only reviewed the actions commit, but here are my comments.
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'll want to add RBAC checks to these actions. For example,
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.
Prefer functional components for new components
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.
Unnecessary span?
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 don't see us passing the event to these callbacks in the implementation. We should take it out if we're not using it.
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.
isVMRunning etc
f0bad12 to
18f90d3
Compare
|
addressed all the comments |
18f90d3 to
60d37f0
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.
| import { K8sKind, PodKind } from '../../../../../public/module/k8s'; | |
| import { K8sKind, PodKind } from '@console/internal/module/k8s'; |
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, fixed.
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.
| import { PodKind } from '../../../../../public/module/k8s'; | |
| import { PodKind } from '@console/internal/module/k8s'; |
60d37f0 to
fbaf526
Compare
|
/ok-to-test |
fbaf526 to
341d43e
Compare
frontend/packages/kubevirt-plugin/src/components/vms/menu-actions.tsx
Outdated
Show resolved
Hide resolved
341d43e to
8010a29
Compare
|
fixed |
8010a29 to
ead6e8a
Compare
ead6e8a to
2ae37fe
Compare
|
added types and new PR dependency |
|
/retest |
2ae37fe to
7e5f703
Compare
|
/retest |
|
/lgtm |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
depends on: