Show VMI details #3949
Show VMI details #3949
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
4 times, most recently
from
09d0362 to
b521dba
Compare
|
/test analyze |
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
from
b521dba to
99a0bb4
Compare
|
/test analyze |
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
from
99a0bb4 to
2332126
Compare
openshift-ci-robot
added
the
needs-rebase
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
2 times, most recently
from
2194862 to
205472c
Compare
There was a problem hiding this comment.
I think we already have this function: Please see findVMPod
There was a problem hiding this comment.
merged findVMPod and findLauncherPod
There was a problem hiding this comment.
maybe it would make sense to enhance the VMLikeEntityKind with VMIKind. WDYT?
There was a problem hiding this comment.
vmi is now a vmilike kind 👍
There was a problem hiding this comment.
you could use getVMLikeModel here if we enhance VMLikeEntityKind
There was a problem hiding this comment.
we should take into account that the data should not be editable in the VMI like boot order/nics etc.
There was a problem hiding this comment.
👍 disabled edit kebabs on nics and discs
There was a problem hiding this comment.
can we rather move this to VirtualMachinesDetailsPage and listen there on what is missing?
There was a problem hiding this comment.
shouldn't this kind be conditional as well?
There was a problem hiding this comment.
The idea of this component is to always point to the .../virtualmachines/... path.
so if someone press the VMI link we want them to always end up in the .../virtualmachines/... page.
There was a problem hiding this comment.
looked at the docs, no need to share paths, yeee :-) , removed the code that share vm and vmi paths 👍
There was a problem hiding this comment.
IMO we should refactor VM statuses. Nevertheless could we separate VMI status and do it in a followup?
There was a problem hiding this comment.
+1 for refactoring, leaving as is for now.
There was a problem hiding this comment.
merged the vm and vmi get status methods
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
from
205472c to
f38743f
Compare
openshift-ci-robot
added
size/XL
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
from
f38743f to
952547b
Compare
openshift-ci-robot
removed
the
needs-rebase
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
2 times, most recently
from
63ede6f to
5d8d8d2
Compare
There was a problem hiding this comment.
can we rename this to isOff? - it is really confusing this way
There was a problem hiding this comment.
| (vm && isWaitingForVMI(vm)) || | |
| (!vmi && vm && isWaitingForVMI(vm)) || |
There was a problem hiding this comment.
IMO isBeingStopped and isCreated (wrong usage now) can be used with just a presence of VMI.
We just need to substitute isVMCreated(vm) with just a presence of VMI and we can check the underlying pods in both these methods
There was a problem hiding this comment.
IMO we should not overuse this type. We should use it in places which can trully handle all 3 types, which is not this case. Can you please make another type for vm and vmi or change it back to VMKind | VMIKind
There was a problem hiding this comment.
although in the future these selectors could handle VMLikeEntityKind and add spec.template when needed
There was a problem hiding this comment.
these selectors would need a second thought - lets not get into this right now
There was a problem hiding this comment.
can we refine the types here and in other places?
There was a problem hiding this comment.
what is wrong with this function? Why can't we keep it ?
There was a problem hiding this comment.
lets make another type for VMKind | VMIKind and use these where needed
you can even do
export type VMILikeEntityKind = VMKind | VMIKind;
export type VMLikeEntityKind = VMILikeEntityKind | TemplateKind;There was a problem hiding this comment.
there are places where we dont expect VMI. What about?
export type VMILikeEntityKind = VMKind | VMIKind;
export type VMLikeEntityKind = VMKind | TemplateKind;
export type VMGenericLikeEntityKind = VMLikeEntityKind | VMILikeEntityKindstill not sure about the naming
thoughts?
There was a problem hiding this comment.
let's please check the ownerReferences first before we show that
There was a problem hiding this comment.
I changes the text to make sense also when this VMI is owned :-)
p.s.
for this PR users should not end up in a VMI details if it's owned because we removed the easy links to it.
There was a problem hiding this comment.
I don't know if this is a good solution. Please consider removing it or making it conditional.
Btw they will end up here quite easily when they click on a link in VM events
There was a problem hiding this comment.
just making sure that this is correct?
There was a problem hiding this comment.
yes, the idea is to have the same page ... for vm and vmi, It do have different path now, but user should see the same breadcrumbs for both ...
p.s.
when changing namespace it will get to the wrong list, but this will have to be looked at at later time.
There was a problem hiding this comment.
for the detail, shouldn't hurt removing the namespace select altogether
There was a problem hiding this comment.
makes sense to me too, this is a console wise change, so not for this PR, I agree we should look how to do this.
There was a problem hiding this comment.
we should lookup the vm if it has one - for the status.
There was a problem hiding this comment.
we should pass down a flag - what kind of page we are showing as both vm and vmi can be present. The user can easily navigate to VMI even if VM is there
There was a problem hiding this comment.
we can show it but it should not be editable
There was a problem hiding this comment.
but it should be at least visible
There was a problem hiding this comment.
why are we removing the VMI instance link? I though Pod link should be removed
There was a problem hiding this comment.
:-) things change ... the reason is that the vm and vmi page show the same data ... so no new info ...
p.s.
leaving it will require the page to know if it's a vm of vmi page ...
There was a problem hiding this comment.
lets please forbid editing for flavor and dedicated resources as well
There was a problem hiding this comment.
btw, does it make sense to create a <Context> for all components to check if it is a vm or vmi to set editability of their fields? E.g. annotations and description should be editable and others most likely not
There was a problem hiding this comment.
It will be nice to know if we are in vm or vmi, but we can do it in a follow up because it will need to be tested and we may introduce new bugs if we do it too quickly.
There was a problem hiding this comment.
getVmiIpAddressesString can be removed now
There was a problem hiding this comment.
menuActions need to be fixed to take VMI into account + also the Consoles
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
from
0cc4846 to
cb94102
Compare
There was a problem hiding this comment.
the vmi check should go inside so we check for the status of the launcher pod
There was a problem hiding this comment.
why do we have to check for the spec?
There was a problem hiding this comment.
I don't know if this is a good solution. Please consider removing it or making it conditional.
Btw they will end up here quite easily when they click on a link in VM events
There was a problem hiding this comment.
but it should be at least visible
There was a problem hiding this comment.
please change the name as it is a duplicate
yaacov
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
2 times, most recently
from
a546fcc to
6bb1d80
Compare
|
/test analyze |
openshift-ci-robot
added
size/XXL
openshift-ci-robot
added
the
needs-rebase
- move around imports to prevent circular dependencies - better naming? - add flavor/workload/os to the vmi when creating the vms - show all VMI details correctly and without editing possibilities (except description) - fix types - do not load unnecesary resources in vm console - fix description editing when empty - do not show empty template link in VMI - unify selectors for VMWrapper and VMIWrapper
atiratree
force-pushed
the
kubevirt-vmi-details-in-vm-page
branch
from
9582e13 to
7e1c078
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jelkosz, yaacov 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 |
openshift-ci-robot
removed
the
needs-rebase
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@yaacov: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
8 participants
Show VMI as type of Virtual Machine
Design:
openshift/openshift-origin-design#310
Screenshot:
VM Details page:
VMI Details page:
VM List page: