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

Fix Proxmox node, name condition. #5108

Merged
merged 2 commits into from
Aug 12, 2022
Merged

Fix Proxmox node, name condition. #5108

merged 2 commits into from
Aug 12, 2022

Conversation

reitermarkus
Copy link
Contributor

SUMMARY

Fix condition which is always false.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

proxmox_kvm

ADDITIONAL INFORMATION

not (node, name) is always false, the correct check is if either node or name is unspecified.

Extracted from #4027.

@ansibullbot
Copy link
Collaborator

cc @Ajpantuso @Thulium-Drake @helldorado @joshainglis @karmab @tleguern
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug cloud module module plugins plugin (any type) labels Aug 11, 2022
russoz
russoz reviewed Aug 11, 2022
View reviewed changes
plugins/modules/cloud/misc/proxmox_kvm.py
@@ -1234,7 +1234,7 @@ def main():
module.exit_json(changed=False, vmid=vmid, msg="VM with vmid <%s> already exists" % vmid)
elif proxmox.get_vmid(name, ignore_missing=True) and not (update or clone):
module.exit_json(changed=False, vmid=proxmox.get_vmid(name), msg="VM with name <%s> already exists" % name)
elif not (node, name):
elif (not node) or (not name):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be overkill, but it could also be expressed as:

Suggested change
elif (not node) or (not name):
elif not all(node, name):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I find the previous version easier to understand since I don't usually write Python.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suit yourself 😁

felixfontein
felixfontein approved these changes Aug 11, 2022
View reviewed changes
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.

I'm both fine with the current version or @russoz's suggestion.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Aug 11, 2022
russoz
russoz reviewed Aug 11, 2022
View reviewed changes
changelogs/fragments/5108-proxmox-node-name-condition.yml Outdated
@@ -0,0 +1,2 @@
deprecated_features:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@felixfontein wouldn't this be a bug fix instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's fixed now, was a copy-paste error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, thanks for spotting that!

russoz
russoz approved these changes Aug 12, 2022
View reviewed changes
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM

@russoz russoz merged commit 0338eb7 into ansible-collections:main Aug 12, 2022
@patchback
Copy link

patchback bot commented Aug 12, 2022 •
edited
Loading

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/0338eb7a7c4f28882ea2df57192b9b8c0093a1ec/pr-5108

Backported as #5110

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Aug 12, 2022
@reitermarkus @patchback
Fix Proxmox node, name condition. (#5108)
0ecd7bc 
* Fix Proxmox `node`, `name` condition.

* Add changelog entry.

(cherry picked from commit 0338eb7)
@patchback patchback bot mentioned this pull request Aug 12, 2022
Merged
@russoz
Copy link
Collaborator

russoz commented Aug 12, 2022

@reitermarkus thanks for the contribution!
@felixfontein thanks for reviewing!

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Aug 12, 2022
felixfontein pushed a commit that referenced this pull request Aug 12, 2022
@patchback @reitermarkus
Fix Proxmox node, name condition. (#5108) (#5110)
5a93168 
* Fix Proxmox `node`, `name` condition.

* Add changelog entry.

(cherry picked from commit 0338eb7)

Co-authored-by: Markus Reiter <me@reitermark.us>
@reitermarkus reitermarkus deleted the proxmox-condition branch August 17, 2022 16:32
@YWatchman
Copy link
Contributor

This broke the update feature using the vmid

  - name: Make sure interface ip config is present.
    proxmox_kvm:
      update: yes
      node: "{{ proxmox_vm_template_target_node }}"
      proxmox_default_behavior: no_defaults
      vmid: "{{ proxmox_vm_create.vmid }}"
      ipconfig: "{{ proxmox_vm_ipconfig }}"

result:

TASK [proxmox-vm : Make sure interface ip config is present.] **********************************************************************************************************************************************
task path: /home/yvan/ansible/roles/proxmox-vm/tasks/create.yml:55
redirecting (type: modules) ansible.builtin.proxmox_kvm to community.general.proxmox_kvm
redirecting (type: modules) community.general.proxmox_kvm to community.general.cloud.misc.proxmox_kvm
fatal: [web0.staging.helpdesk.......nl -> localhost]: FAILED! => {
    "changed": false
}

MSG:

node, name is mandatory for creating/updating vm

It worked until I updated the ansible (collection) version.

@YWatchman YWatchman mentioned this pull request Sep 1, 2022
Merged
@felixfontein
Copy link
Collaborator

#5206 fixes this.

@patchback patchback bot mentioned this pull request Sep 12, 2022
Merged
bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
@reitermarkus
Fix Proxmox node, name condition. (ansible-collections#5108)
4731fe0 
* Fix Proxmox `node`, `name` condition.

* Add changelog entry.
bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
@reitermarkus
Fix Proxmox node, name condition. (ansible-collections#5108)
8da901c 
* Fix Proxmox `node`, `name` condition.

* Add changelog entry.
@trux-bot trux-bot bot mentioned this pull request Nov 7, 2022
Merged
1 task
@lumiere-bot lumiere-bot bot mentioned this pull request Nov 7, 2022
Merged
1 task
This was referenced Nov 20, 2022
Closed
Merged
Closed
Closed
Closed
Closed
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Dec 1, 2022
Open
Closed
@renovate renovate bot mentioned this pull request Dec 14, 2022
Closed
1 task
@renovate renovate bot mentioned this pull request Jan 1, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

@russoz russoz russoz approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug cloud module module plugins plugin (any type)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@reitermarkus @ansibullbot @russoz @YWatchman @felixfontein