List VMIs in the VM list view #3841

yaacov · 2019-12-31T10:12:38Z

List VMIs in the VM list view

We decided to expose VMIs to users, this PR expose the the VMI in the VM list page.

list VMIs in the "Virtual machines" list.
Add created, node and IP columns.
Update the "running" icon to align with console "running" icon.
Sort by status.

Refs:
https://issues.redhat.com/browse/KNIP-1165
openshift/openshift-origin-design#310

yaacov · 2019-12-31T10:13:29Z

@irosenzw @glekner please review

cc:// @yfrimanm

glekner · 2020-01-01T14:45:29Z

/lgtm

openshift-bot · 2020-01-01T14:58:35Z

/retest

Please review the full test history for this PR and help us cut down flakes.

yaacov · 2020-01-01T15:03:12Z

/hold

until e2e test is fixed ...

matthewcarleton · 2020-01-02T17:28:29Z

@yaacov thanks for doing this!
It's nice to see the option implemented.
I think biggest issues I see with this option are:

If the user goes looking for a VMI will they find it easily? It could be buried in a long list.
Is it easy for users to understand what a VMI is and how it relates to VMs?
In this scenario that VMI may or may not have an owner. I seems useful to indicate when there is no owner in case the VMI should not exist.

atiratree · 2020-01-02T17:38:41Z

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

sort is not working

hmm, ill look at it when implementing #3841 (comment) , sorting by nodes is much more tricky :-)

atiratree · 2020-01-02T17:42:47Z

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

is this really the best icon for running? Sync icon?

Yes, but It's what the rest of the console is using ... :-(

https://github.com/openshift/console/blob/master/frontend/packages/dev-console/src/components/pipelines/detail-page-tabs/pipeline-details/StatusIcon.tsx#L22

atiratree · 2020-01-02T17:44:41Z

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

I guess, sorting by node could be useful as well

It's more tricky than you think :-)
VM kind has no direct reference to node ... will look how to do it nicely.

atiratree · 2020-01-02T17:51:15Z

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

maybe we could make isVM variable and use it in the conditionals bellow?

👍 thanks

spadgett · 2020-01-02T23:21:33Z

#3847 has fixed the e2e tests, but leaving the hold since there are pending review comments

jelkosz · 2020-01-06T08:04:38Z

now it should pass
/retest

yaacov · 2020-01-08T08:45:15Z

@suomiy hi, i addressed the issues you raised, please re review.

yaacov · 2020-01-08T11:03:25Z

/hold cancel
it was for the e2e tests, we can remove it now

atiratree

looks good and works :)

atiratree · 2020-01-08T10:16:02Z

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

can we use a better name?

atiratree · 2020-01-08T10:28:55Z

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

can be switched with line above to have only one call of getBasicID

nice, missed that ...

atiratree · 2020-01-08T10:29:03Z

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

we should probably wait for vms to load first before we show vmis to prevent flickering

atiratree · 2020-01-08T10:29:56Z

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

IMO it would be more readable to inline the function body here

atiratree · 2020-01-08T10:42:36Z

frontend/packages/kubevirt-plugin/src/statuses/vm/vm.ts

these little helpers around the statuses are getting out of hand. We should refactor all of it and use ValueEnum for that (example usage in e.g. StorageUISource, DiskType)

I guess you don't want to do it in this PR right? >.<

atiratree · 2020-01-08T10:50:23Z

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

we already looked for the status in the flatten function, so we don't have to id here and we can just pass it.

atiratree · 2020-01-08T10:54:49Z

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

can we name it vmMenuActions and vmiMenuActions?

we could also move the vmiMenuActions to standalone file so they can be exportable in the future.

renamed and moved to menu-actions for now ...

atiratree · 2020-01-08T10:58:39Z

frontend/packages/kubevirt-plugin/src/statuses/vm/vm.ts

we should rather just pass the status as an argument and move this function to ./constants.ts

atiratree · 2020-01-08T11:10:05Z

frontend/packages/kubevirt-plugin/src/statuses/vm/vm.ts

Isn't it going to be confusing that the sort behaves differently than the visual perspective?

I get, that the categories might be useful. Anyway, we should rather name it getVMStatusSortString.

side note: we could have the categories present in ValueEnum

renamed and moved to constants

yaacov · 2020-01-08T15:05:12Z

/test analyze

atiratree · 2020-01-08T15:08:08Z

looks good, but still missing #3841 (comment)

…te running icon Signed-off-by: yaacov <kobi.zamir@gmail.com>

openshift-bot · 2020-01-10T07:00:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T07:12:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T08:04:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T08:17:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T09:09:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T09:22:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T10:14:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T11:19:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T11:32:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T12:37:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T12:50:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T13:56:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T15:12:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T15:38:25Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T16:43:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T16:56:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-10T18:02:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2020-01-10T18:27:12Z

/hold
the status for e2e-gcp-console is incorrect. this hasn't passed CI yet

spadgett · 2020-01-10T19:03:52Z

/test e2e-gcp-console

spadgett · 2020-01-10T19:04:33Z

/hold cancel

spadgett · 2020-01-10T19:10:10Z

/hold
let's make sure the test runs to completion

spadgett · 2020-01-11T17:58:31Z

/hold cancel
/retest

openshift-bot · 2020-01-11T18:43:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 31, 2019

openshift-ci-robot requested review from pcbailey and rhrazdil December 31, 2019 10:12

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin labels Dec 31, 2019

yaacov mentioned this pull request Dec 31, 2019

kubevirt: add list columns #3821

Closed

3 tasks

openshift-ci-robot assigned glekner Jan 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 1, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 1, 2020

yaacov mentioned this pull request Jan 2, 2020

Surfacing VMIs in the UI openshift/openshift-origin-design#310

Merged

atiratree suggested changes Jan 2, 2020

View reviewed changes

yaacov force-pushed the kubevirt-add-vmis-to-vm-list branch from 5e51e50 to b97ff4b Compare January 8, 2020 08:40

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2020

atiratree suggested changes Jan 8, 2020

View reviewed changes

yaacov force-pushed the kubevirt-add-vmis-to-vm-list branch 3 times, most recently from 21643c9 to 938ff62 Compare January 8, 2020 12:37

List VMIs in the VM list view, add created, node and ip columns, upda…

b8a8928

…te running icon Signed-off-by: yaacov <kobi.zamir@gmail.com>

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2020

openshift-merge-robot merged commit 0ddf1e6 into openshift:master Jan 11, 2020

yaacov mentioned this pull request Jan 13, 2020

createLookup take firehose object not list ... #3926

Merged

yaacov mentioned this pull request Sep 9, 2020

Bug 1873402: Add deleting status #6565

Merged

List VMIs in the VM list view #3841

List VMIs in the VM list view #3841

Uh oh!

Conversation

yaacov commented Dec 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaacov commented Dec 31, 2019

Uh oh!

glekner commented Jan 1, 2020

Uh oh!

openshift-bot commented Jan 1, 2020

Uh oh!

yaacov commented Jan 1, 2020

Uh oh!

matthewcarleton commented Jan 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Jan 2, 2020

Uh oh!

jelkosz commented Jan 6, 2020

Uh oh!

yaacov commented Jan 8, 2020

Uh oh!

yaacov commented Jan 8, 2020

Uh oh!

atiratree left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaacov commented Dec 31, 2019 •

edited

Loading