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.

Sign up for GitHub

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

Jump to bottom

vmware_vm_config_option: add more properties in output when getting VM configurations #1202

Merged
merged 5 commits into from
Mar 25, 2022
Merged

vmware_vm_config_option: add more properties in output when getting VM configurations #1202

merged 5 commits into from
Mar 25, 2022

Conversation

Tomorrow9
Copy link
Collaborator

@Tomorrow9 Tomorrow9 commented Jan 26, 2022 •
edited by goneri
Loading

Depends-On: ansible/ansible-zuul-jobs#1420

Signed-off-by: dianew [email protected]

SUMMARY

Add supported disk controller types, supported network adapter types, supported USB controller types in the recommended_config_options output.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

vmware_vm_config_option

ADDITIONAL INFORMATION
ok: [localhost] => {
    "vm_default_config": {
        "changed": false,
        "failed": false,
        "instance": {
            "guest_id": "amazonlinux2_64Guest",
            "hardware_version": "vmx-15",
            "recommended_config_options": {
                "default_cdrom_controller": "sata",
                "default_disk_controller": "paravirtual",
                "default_ethernet": "vmxnet3",
                "default_firmware": "efi",
                "default_secure_boot": false,
                "default_usb_controller": "",
                "guest_fullname": "Amazon Linux 2 (64-bit)",
                "rec_cpu_cores_per_socket": 2,
                "rec_cpu_socket": 1,
                "rec_disk_mb": 16384,
                "rec_memory_mb": 2048,
                "rec_persistent_memory": 8192,
                "rec_vram_kb": 8192,
                "support_disk_controller": [
                    "lsilogic",
                    "paravirtual",
                    "lsilogicsas",
                    "sata",
                    "nvme"
                ],
                "support_ethernet_card": [
                    "vmxnet3",
                    "e1000e",
                    "sriov"
                ],
                "support_min_persistent_mem_mb": 4,
                "support_persistent_memory": true,
                "support_secure_boot": true,
                "support_tpm_20": false,
                "support_usb_controller": [
                    "usb2",
                    "usb3"
                ]
            }
        }
    }
}

@Tomorrow9 Tomorrow9 changed the title add support disk controller properties vmware_vm_config_option: add more properties in output when getting VM configurations Jan 26, 2022
@softwarefactory-project-zuul
Copy link

Build succeeded.

@mariolenz
Copy link
Collaborator

@Tomorrow9 I'm not sure if we can change the result from for example Recommended config options to recommended_config_options or Default CDROM controller to default_cdrom_controller. This might be considered a breaking change. You see, a playbook that uses this this module and then wants to use Default disk controller will fail because this is now called default_disk_controller. (The same applies to the other changes like Supported hardware versions -> supported_hardware_versions.)

I can see why you want to change this, but I think we should wait until the next major release.

However, I think it wouldn't hurt to return the result in both the old and the new format. In version 3, we can then remove the old format. This would allow existing playbooks to work the old way, while you can already use the new keys.

I don't think we need to introduce an old format style for the stuff that you've added, though. That is, we don't need Supported Ethernet Cards or similar in addition to support_ethernet_card.

@Tomorrow9
Copy link
Collaborator Author

Tomorrow9 commented Feb 14, 2022 •
edited
Loading

@Tomorrow9 I'm not sure if we can change the result from for example Recommended config options to recommended_config_options or Default CDROM controller to default_cdrom_controller. This might be considered a breaking change. You see, a playbook that uses this this module and then wants to use Default disk controller will fail because this is now called default_disk_controller. (The same applies to the other changes like Supported hardware versions -> supported_hardware_versions.)

I can see why you want to change this, but I think we should wait until the next major release.

Yeah, you're right, in our experience using this module, it seems more convenient and following the convention using default_cdrom_controller instead of Default CDROM controller.

However, I think it wouldn't hurt to return the result in both the old and the new format. In version 3, we can then remove the old format. This would allow existing playbooks to work the old way, while you can already use the new keys.

Do you think the result containing both formats is good? Then I can change it back and as you said change it to "xx_xx_xx" format in version 3.

I don't think we need to introduce an old format style for the stuff that you've added, though. That is, we don't need Supported Ethernet Cards or similar in addition to support_ethernet_card.

In our playbooks, supported disks controllers or ethernet types for one gosID are required, so I added these in the returned results.

Thanks for reviewing this.

@mariolenz
Copy link
Collaborator

However, I think it wouldn't hurt to return the result in both the old and the new format. In version 3, we can then remove the old format. This would allow existing playbooks to work the old way, while you can already use the new keys.

Do you think the result containing both formats is good? Then I can change it back and as you said change it to "xx_xx_xx" format in version 3.

Well, it wouldn't break existing playbooks if you keep the old format. And having both shouldn't break anything, either.

Could you please share your opinion on this @sky-joker? I'd like to hear what you think about this.

@mariolenz
Copy link
Collaborator

@Tomorrow9 Are you still working on this?

@Tomorrow9
Copy link
Collaborator Author

@Tomorrow9 Are you still working on this?

Yeah, if anybody else does not have other comments, then I'll change the existing output back in the old format, and add new output items in new format. Thanks.

@Tomorrow9 Tomorrow9 force-pushed the fix_vm_config_option branch from 7bc24a5 to ee9cdbf Compare March 15, 2022 08:18
@Tomorrow9
Copy link
Collaborator Author

@mariolenz do you have any other comments on this change? Thanks.

@softwarefactory-project-zuul
Copy link

Build failed.

@mariolenz
Copy link
Collaborator

@mariolenz do you have any other comments on this change? Thanks.

Yes, one: The two CI jobs ansible-test-units-community-vmware-python37 and ansible-test-units-community-vmware-python38 fail with fixture 'patch_ansible_module' not found. I've seen this in other PRs, too. Maybe @goneri has an idea why this happens.

I'll have another look at this when the CI is running again.

@mariolenz
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@mariolenz
Copy link
Collaborator

mariolenz commented Mar 16, 2022 •
edited
Loading

@Tomorrow9

The two CI jobs ansible-test-units-community-vmware-python37 and ansible-test-units-community-vmware-python38 fail with fixture 'patch_ansible_module' not found.

Probably pytest-dev/pytest#9767.

@softwarefactory-project-zuul
Copy link

Build failed.

@mariolenz
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@mariolenz
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@mariolenz
Copy link
Collaborator

recheck

@mariolenz mariolenz mentioned this pull request Mar 22, 2022
Closed
@goneri
Copy link
Member

goneri commented Mar 24, 2022

recheck

@ansible-collections ansible-collections deleted a comment from mariolenz Mar 24, 2022
@ansible-collections ansible-collections deleted a comment from mariolenz Mar 24, 2022
@ansible-collections ansible-collections deleted a comment from mariolenz Mar 24, 2022
@ansible-collections ansible-collections deleted a comment from Tomorrow9 Mar 24, 2022
@ansible-collections ansible-collections deleted a comment from mariolenz Mar 24, 2022
@softwarefactory-project-zuul
Copy link

Build succeeded.

mariolenz
mariolenz approved these changes Mar 25, 2022
View reviewed changes
Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mariolenz
Copy link
Collaborator

regate

@mariolenz mariolenz removed the mergeit label Mar 25, 2022
@mariolenz mariolenz closed this Mar 25, 2022
@mariolenz mariolenz reopened this Mar 25, 2022
@softwarefactory-project-zuul
Copy link

Build failed.

@mariolenz
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@mariolenz
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link

Build succeeded.

@mariolenz mariolenz added mergeit and removed mergeit labels Mar 25, 2022
@softwarefactory-project-zuul
Copy link

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit e0f14ba into ansible-collections:main Mar 25, 2022
@mariolenz mariolenz mentioned this pull request Mar 26, 2022
Closed
@mariolenz
Copy link
Collaborator

@Tomorrow9 I've added #1268 to milestone 3.0.0 so we don't forget to change the result.

@Tomorrow9
Copy link
Collaborator Author

@Tomorrow9 I've added #1268 to milestone 3.0.0 so we don't forget to change the result.

Ok, thanks a lot.

@mariolenz mariolenz mentioned this pull request Sep 9, 2022
Merged
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Sep 10, 2022
@mariolenz
vmware_vm_config_option: Change item names in result from strings joi…
75df712 
…ned with spaces to strings joined with underlines (#1457)

vmware_vm_config_option: Change item names in result

SUMMARY
Change item names in result from strings joined with spaces to strings joined with underlines, e.g. Guest fullname will be changed to guest_fullname.
Fixes #1268
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
vmware_vm_config_option
ADDITIONAL INFORMATION
#1202
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariolenz mariolenz mariolenz approved these changes

Assignees
No one assigned
Labels
mergeit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tomorrow9 @mariolenz @goneri