-
Notifications
You must be signed in to change notification settings - Fork 28
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
Bug 2181920: Display "UEFI" for Boot mode for "efi: {}" in yaml #1287
Bug 2181920: Display "UEFI" for Boot mode for "efi: {}" in yaml #1287
Conversation
@hstastna: This pull request references Bugzilla bug 2181920, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
@hstastna: This pull request references Bugzilla bug 2181920, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
1 similar comment
@hstastna: This pull request references Bugzilla bug 2181920, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
@avivtur @metalice @pcbailey @upalatucci @vojtechszocs please review |
@hstastna: This pull request references Bugzilla bug 2181920, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
||
import { BootloaderOptionValue } from './constants'; | ||
|
||
export const isObjectEmpty = (obj: object): boolean => isEmpty(obj) && obj !== undefined; |
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.
What about case null? Is possible? i think isEmpty(obj) && obj can work also?
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.
In the place I am using this function, null isn't possible. But if someone decided to use this somewhere else, probably isEmpty(obj) && obj
is better to use.
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.
Can you explain to me why just isEmpty(obj) is not enough? it should return true if it's either one of these:
null / undefind / [] / {} / ''
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.
Because we don't want to receive true if the object is null or undefined. We want to end up here in case of null or undefined. And if vm?.spec?.template?.spec?.domain?.firmware?.bootloader?.efi
is equal to {}
, it means secure boot enabled..
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.
so efi: {}
and efi: { secureBoot: true }
are equivalent right? and if efi
does not exist we want bios?
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, they both refer to secure boot enabled displayed as "UEFI (secure)" option in the UI. And yes, if no efi, then show "BIOS". This was the original implementation at least. I was not dealing with this third option too much, the main problem was the secure boot option. I've just added this to the description of the PR to avoid future confusions.
/retest |
@hstastna: This pull request references Bugzilla bug 2181920, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
1 similar comment
@hstastna: This pull request references Bugzilla bug 2181920, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
2b8c089
to
ced3fd7
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.
looks good just one nit and I think it's good to go
vmDraft.spec.template.spec.domain.features.smm = { enabled: true }; | ||
|
||
switch (firmwareBootloader) { | ||
case 'uefi': |
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.
can you please add these hard-coded constants into a constants file? uefi
eufiSecure
..
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.
Seems that I am doing more refactoring in this PR than bug fixing ...
712263c
to
520a507
Compare
This is a followup of kubevirt-ui#1236 introducing the change that "efi: { secureBoot: false }" was written to VM's yaml when changing Boot mode field value to "UEFI", because that was correct representation of "UEFI" boot mode (secure boot disabled). In this commit, the following remaining issues are fixed: - for existing VM: -- it shows "UEFI" for Boot mode field in Details for "efi: {}" in the yaml's bootloader section - in Create VM wizard: -- it shows "UEFI" for Boot mode field in Review and create VirtualMachine screen for "efi: {}" in the yaml's bootloader section - for existing template: -- it shows "UEFI" for Boot mode field in Details for "efi: {}" in the yaml's bootloader section -- after changing Boot mode field to "UEFI", "efi: {}" occurs in the yaml (this doesn't happen for existing VMs or when creating a VM) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2181920
520a507
to
4316e3e
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avivtur, hstastna 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 |
@hstastna: All pull requests linked via external trackers have merged: Bugzilla bug 2181920 has been moved to the MODIFIED state. 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. |
📝 Description
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2181920
This is a followup of #1236 introducing the change that
efi: { secureBoot: false }
was written to VM's yaml when changing Boot mode field value to "UEFI", because that was correct representation of "UEFI" boot mode (secure boot disabled).In this PR, the following remaining issues are fixed:
efi: {}
in the yaml's bootloader sectionefi: {}
in the yaml's bootloader sectionefi: {}
in the yaml's bootloader sectionefi: {}
occurs in the yaml (this doesn't happen for existing VMs or when creating a VM)Part of this PR is also a bit of refactoring - removing duplicated code for updating the VM with selected boot mode data.
The result of refactoring is new
updatedVMBootMode
function that returns the updated VM object.Expected results:
The correct value in the yaml after selecting UEFI should be
efi: { secureBoot: false }
. Also forefi: {}
in the YAML (as many default templates come with this), secure boot is enabled and "UEFI (secure)" should be displayed in Boot mode field in the UI.Notes about the moot modes:
Both
efi: {}
andefi: { secureBoot: true }
in the yaml are equivalent (at least now, maybe this will change) and refer to secure boot enabled displayed as "UEFI (secure)" option in the UI. If noefi
present, then "BIOS" is displayed as a boot mode, in the Details tab. This was the original implementation at least. I was not dealing with this third option too much, the main problem was the secure boot option, that was enabled while the UI showed just "UEFI".How to reproduce the bug:
#1236 (comment)
Note:
It is possible this issue will be fixed by backend eventually, probably the way that
efi: {}
in the yaml won't represent secure boot anymore (but it's possible they will come up with something else). But till then, we need this PR to make this work as expected.🎥 Screenshots
Example of a VM yaml:
Before:
"UEFI" is displayed for Boot mode field in VM Details for
efi: {}
(see the yaml example above):After changing Boot mode value to "UEFI",
efi: {}
occurs in the VM Template's yaml:After:
"UEFI (secure)" is displayed for Boot mode field in VM Details for
efi: {}
as expected:After changing Boot mode value to "UEFI",
efi: { secureBoot: false }
occurs in the VM Template's yaml: