-
Notifications
You must be signed in to change notification settings - Fork 616
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 Status column to VmList #1689
Conversation
Hi @mareklibra. 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. 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. |
frontend/packages/_plugin-style.scss
Outdated
@@ -0,0 +1,4 @@ | |||
/* Styles of all active plugins */ | |||
|
|||
@import "kubevirt-plugin/src/style"; |
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.
Plugin's entrypoint can import style, so this will be removed
{/* TODO(mlibra): migrate VM status in a follow-up | ||
<ColHead {...props} className={mainRowSize} sortField="spec.running">State</ColHead> | ||
*/} | ||
<ColHead {...props} className={mainRowSize} sortField="spec.running"> |
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.
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.
ok, this is wip in #1758
@@ -0,0 +1,60 @@ | |||
import { K8sResourceKind, ObjectMetadata } 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.
Nit-pick: I'd use a more specific file name, i.e. k8s-types
or similar.
Rebased as master and #1682 were changed. |
/ok-to-test |
@spadgett , can you please have a look? |
|
||
const filters = [ | ||
{ | ||
type: kubevirtTableFilters.VmStatus.type, |
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.
Wouldn't it be simpler to define whole vmStatusFilter
object in kubevirtTableFilters
, export it and just pass it in rowFilters prop?
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.
See 768bb67
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.
good idea, thanks. I've moved Filter
type to @console/shared
.
}; | ||
|
||
const VMRowFirehose = ({ obj: vm }: React.ComponentProps<typeof ResourceRow>) => { |
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 that once this #1758 merges, it will be possible to pass resources to MultiListPage so you could use that to fetch migrations and pods and avoid connecting each row separately.
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, that will be much easier. I don't have any numbers, but I believe this is not a blocker for this PR as the performance is not significantly hit by wrapping every 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.
I would prefer to wait for #1758 to merge (probably today) as the new approach is more performant and does use only few websockets instead of n
.
/test e2e-aws |
Rebased as #1761 is merged now. |
/test frontend |
/test frontend |
all required PRs are now merged and i also rebased on master. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mareklibra, 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 |
The
State
column is added intoVmList
page.Depends on:
TODO: