Skip to content
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

Added bios_type parameter to the ovirt_vm module #56918

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@smapjb
Copy link

commented May 24, 2019

SUMMARY

I was not able to set the bios type to UEFI from the ovirt_vm module. So I added that feature. See documentation snippet in the module.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

oVirt module

ADDITIONAL INFORMATION
@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/ovirt/ovirt_vm.py:1332:86: bad-whitespace Exactly one space required after comma                 otypes.Bios(boot_menu=otypes.BootMenu(enabled=self.param('boot_menu')),type=otypes.BiosType(self.param('bios_type')))                                                                                       ^

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/ovirt/ovirt_vm.py:235:119: W291 trailing whitespace
lib/ansible/modules/cloud/ovirt/ovirt_vm.py:1332:87: E231 missing whitespace after ','

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/cloud/ovirt/ovirt_vm.py:0:0: E324 Argument 'bios_type' in argument_spec defines default as ('i440fx_sea_bios') but documentation defines default as (None)
lib/ansible/modules/cloud/ovirt/ovirt_vm.py:0:0: E326 Argument 'bios_type' in argument_spec defines choices as (['i440fx_sea_bios', 'q35_ovmf', 'q35_sea_bios', 'q35_secure_boot']) but documentation defines choices as ([])

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/ovirt/ovirt_vm.py:235:119: W291 trailing whitespace

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/cloud/ovirt/ovirt_vm.py:0:0: E324 Argument 'bios_type' in argument_spec defines default as ('i440fx_sea_bios') but documentation defines default as (None)
lib/ansible/modules/cloud/ovirt/ovirt_vm.py:0:0: E326 Argument 'bios_type' in argument_spec defines choices as (['i440fx_sea_bios', 'q35_ovmf', 'q35_sea_bios', 'q35_secure_boot']) but documentation defines choices as ([])

click here for bot help

@machacekondra
Copy link
Contributor

left a comment

Thanks a lot! shipit

@ansibot ansibot removed the needs_triage label May 27, 2019

@mwperina
Copy link
Contributor

left a comment

shipit

@ansibot ansibot added shipit and removed community_review labels May 27, 2019

@resmo
Copy link
Member

left a comment

needs_info

otypes.Bios(boot_menu=otypes.BootMenu(enabled=self.param('boot_menu')))
) if self.param('boot_menu') is not None else None,
otypes.Bios(boot_menu=otypes.BootMenu(enabled=self.param('boot_menu')), type=otypes.BiosType(self.param('bios_type')))
) if self.param('boot_menu') is not None or self.param('bios_type') is not None else None,

This comment has been minimized.

Copy link
@resmo

resmo Jun 6, 2019

Member

the logic of self.param('boot_menu') is not None is always true because this PR changes its default to false. Changing the default of boot_menu seems unnecessary for this logic. Or did I miss anything?

This comment has been minimized.

Copy link
@smapjb

smapjb Jun 6, 2019

Author

I set a default boot_menu for the case when only the bios_type parameter is provided. However there is a consequence to this that I have just discovered.

If you provision a machine say with bios_type or boot_menu set, and then you subsequently leave them out of a task, e.g to 'suspend' a machine say, then the machine config is marked as changed because they revert to the default values.

I need to spend another afternoon looking through the 'marked as changed' logic. At the moment if you set a boot_menu or bios_type option you have to include that in all subsequent tasks that use the ovirt_vm module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.