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 conditional to only collect qmpstatus on qemu VMs #4816

Merged
merged 10 commits into from
Jun 11, 2022
Merged

Added conditional to only collect qmpstatus on qemu VMs #4816

merged 10 commits into from
Jun 11, 2022

Conversation

Thulium-Drake
Copy link
Contributor

SUMMARY

This fixes an issue with LXC containers after #4723 was implemented. I completely missed this when reviewing it and the unit tests didn't cover it :-)

I'm not too sure however on how to update the unit tests in order to fix this (yet, but it's late, unless I come up with an idea earlier then someone else, I'll adjust the tests as well :-) )

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

proxmox inventory

ADDITIONAL INFORMATION

Running the module without this task results in the errors below:

[WARNING]:  * Failed to parse /opt/ansible/projects/element-networks/inventory/04_nc_proxmox.yml with
ansible_collections.community.general.plugins.inventory.proxmox plugin: 'qmpstatus'
  File "/usr/lib/python3/dist-packages/ansible/inventory/manager.py", line 290, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/opt/ansible/projects/element-networks/collections/ansible_collections/community/general/plugins/inventory/proxmox.py", line 656, in parse
    self._populate()
  File "/opt/ansible/projects/element-networks/collections/ansible_collections/community/general/plugins/inventory/proxmox.py", line 597, in _populate
    name = self._handle_item(node['node'], ittype, item)
  File "/opt/ansible/projects/element-networks/collections/ansible_collections/community/general/plugins/inventory/proxmox.py", line 507, in _handle_item
    self._get_vm_status(properties, node, vmid, ittype, name)
  File "/opt/ansible/projects/element-networks/collections/ansible_collections/community/general/plugins/inventory/proxmox.py", line 446, in _get_vm_status
    properties[self._fact('qmpstatus')] = ret['qmpstatus']

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug inventory inventory plugin plugins plugin (any type) small_patch Hopefully easy to review traceback labels Jun 9, 2022
@ilijamt
Copy link
Contributor

ilijamt commented Jun 9, 2022

@Thulium-Drake do you think maybe we can just use a default value if not set instead of a vm type check. As qmpstatus doesn't mean it's set even if you have QEMU machine, it will only be set when there are facts.

Yeah, weird that the tests missed it.

@ilijamt
Copy link
Contributor

ilijamt commented Jun 9, 2022

The tests missed the issue because we mock the _get_json.

inventory._get_json = mocker.MagicMock(side_effect=get_json)

This will require a bit more work to add tests for this.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Could you please add a changelog fragment? Thanks.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Jun 10, 2022
@ansibullbot ansibullbot removed the small_patch Hopefully easy to review label Jun 10, 2022
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 10, 2022
@Thulium-Drake
Copy link
Contributor Author

For some reason I run into some weird errors when trying the unit tests on my laptop, but I made some changes to them. It should now 'just' collect statuses from the mocked API instead of noop-ing :-)

@ansibullbot ansibullbot added tests tests unit tests/unit and removed ci_verified Push fixes to PR branch to re-run CI labels Jun 10, 2022
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 10, 2022
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug inventory inventory plugin plugins plugin (any type) shipit tests tests traceback unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants