Show VM name in export status #9145
Conversation
kubevirt-bot
added
release-note
tests/storage/export.go
Outdated
| } | ||
| return export.Status.VirtualMachineName | ||
| }, 30*time.Second, time.Second).Should(Equal(name)) | ||
| return export |
There was a problem hiding this comment.
Correct, but I like to return the updated objects in these types of functions so it can be passed into another function without having to re-read it.
There was a problem hiding this comment.
Yeah understood but until it's actually used it seems odd to include it.
| return "" | ||
| } | ||
| return export.Status.VirtualMachineName | ||
| }, 30*time.Second, time.Second).Should(Equal(name)) |
There was a problem hiding this comment.
nit - This could be a single bool return.
return export.Status != nil && export.Status.VirtualMachineName == name
There was a problem hiding this comment.
But it is expecting a string not a bool.
There was a problem hiding this comment.
I don't like making the Eventually expect a bool, the way it is written now, it prints the name (or blank if export.Status == nil) from the status object. If written your way I won't get that feedback.
There was a problem hiding this comment.
Yeah that's fair, more feedback on failure is always useful, apologies.
|
/lgtm I'd rather the unused return was removed but otherwise this lgtm. |
For display purposes it is handy to have the VM name of the VM/VMSnapshot available in the export status. Signed-off-by: Alexander Wels <awels@redhat.com>
awels
force-pushed
the
add_vm_name_export
branch
from
4218c6a
to
446cc1f
Compare
|
Took the return out, it is not that big of a deal to me. |
|
/lgtm |
|
/retest-required |
| // VirtualMachineName shows the name of the source virtual machine if the source is either a VirtualMachine or | ||
| // a VirtualMachineSnapshot. This is mainly to easily identify the source VirtualMachine in case of a | ||
| // VirtualMachineSnapshot | ||
| VirtualMachineName string `json:"virtualMachineName,omitempty"` |
There was a problem hiding this comment.
I think this type should probably be *string. Will be unset in the case of PVC source
There was a problem hiding this comment.
Good point, I will make that change.
There was a problem hiding this comment.
Does that doc need to be updated as +optional and omitempty work fine with empty string values? I get that using a pointer lets us check for unset values but we'd expect the name here to be empty or set?
Anyway we should really have some kind of lint rule somewhere to enforce the use of pointers for strings if we want things to be consistent, just above this ServiceName is also optional and not a pointer.
There was a problem hiding this comment.
So with just a string we overload "" as does not exist, even though blank might be a valid value. With *string we can use nil as does not exist, and blank is just another possible value. The difference is mostly academic, but in general the rule is, if you have an optional field, it should be a pointer.
There was a problem hiding this comment.
Yeah understood, my point was that "" isn't a valid name for a VirtualMachine so we could use it to mean unset here. Either way using pointers for optional attributes is definitely something we can check and enforce in the codebase but that's for another PR, apologies for the noise!
There was a problem hiding this comment.
@lyarwood good point, ServiceName maybe shouldn't be optional. There will always be a service associated to a VMExport and it's name is known as soon as the VMExport is created
What makes VirtualMachineName different from ServiceName is that VirtualMachineName will only be set for certain VMExports, not all
awels
force-pushed
the
add_vm_name_export
branch
from
2c1791c
to
8cfc70c
Compare
|
/retest-required |
|
Could you fix up 8cfc70c - somehow the subject and signed-off lines have swicthed? |
Signed-off-by: Alexander Wels <awels@redhat.com>
awels
force-pushed
the
add_vm_name_export
branch
from
8cfc70c
to
acae506
Compare
|
Not sure how that happened fixed. Note it looks like the 1.26 storage lane has a permanent failure on some PSA pod. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks 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 |
kubevirt-bot
added
the
approved
|
/retest-required |
6 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
@awels: The following test failed, say
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. I understand the commands that are listed here. |
|
/test pull-kubevirt-e2e-k8s-1.24-sig-network |
|
/cherrypick release-0.59 |
|
@awels: #9145 failed to apply on top of branch "release-0.59": In response to this:
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. |
Show VM name in export status
Show VM name in export status Signed-off-by: Alexander Wels <awels@redhat.com>
Signed-off-by: Alexander Wels awels@redhat.com
What this PR does / why we need it:
For display purposes it is handy to have the VM name of the VM/VMSnapshot available in the export status. Added a new field containing the name and populating it during the standard reconcile loop.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: