[ovirt_disks] Check if value provided for vm_id and vm_name instead of checking for key

[fabianvf]

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

ovirt_disks

ANSIBLE VERSION

ansible 2.2.0.0
  config file = /home/fabian/code/redhat/rhci/ansible-ovirt/ansible.cfg
  configured module search path = Default w/o overrides

SUMMARY

I wanted to create disks that were unattached from a VM, so I used a configuration like this:

- name: create disk
      ovirt_disks:
        name: my_disk
        state: present
        format: cow
        size: 15GiB
        auth: '{{ ovirt_auth }}'
        storage_domain: 'my_data_storage'
        interface: virtio
        wait: true

but this threw an error saying that the VM could not be found. I looked into the code and noticed that the check for whether or not vm information was provided was this:

'vm_id' in module.params or 'vm_name' in module.params and state != 'absent'

which would only check the keys of module.params, which will always have vm_id and vm_name in it. Simplified example of the issue below:

my_dict = {'a': None}
'a' in my_dict  # True
'a' in my_dict.keys()  # True, equivalent to above

I replaced the 'vm_id' in module.params with module.params.get('vm_id') is not None, which should be False when vm_id is not provided by the user. I used is not None rather than relying on python false-iness because I wasn't sure if disk names like 0 or empty string should count as the user not providing them.

Before:

ansible-playbook -i 'x1.example.org,' disk_test.yml --private-key=`s vagrant`                                   git:master

PLAY [all] *********************************************************************

TASK [setup] *******************************************************************
ok: [x1.example.org]

TASK [do the ovirt auth thing] *************************************************
ok: [x1.example.org]

TASK [create disk] *************************************************************
fatal: [x1.example.org]: FAILED! => {"changed": false, "failed": true, "msg": "VM don't exists, please create it first."}
        to retry, use: --limit @/home/fabian/code/redhat/rhci/ansible-ovirt/disk_test.retry

PLAY RECAP *********************************************************************
x1.example.org             : ok=2    changed=0    unreachable=0    failed=1

After:

ansible-playbook -i 'x1.example.org,' disk_test.yml --private-key=`s vagrant`                                   git:master

PLAY [all] *********************************************************************

TASK [setup] *******************************************************************
ok: [x1.example.org]

TASK [do the ovirt auth thing] *************************************************
ok: [x1.example.org]

TASK [create disk] *************************************************************
changed: [x1.example.org]

PLAY RECAP *********************************************************************
x1.example.org             : ok=3    changed=1    unreachable=0    failed=0

[gregdek]

Thanks @fabianvf. To the current maintainers, @machacekondra please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit', 'needs_revision' or 'close_me' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

[machacekondra]

Thank you Fabian. shipit

[machacekondra]

Just FYI, it's not possible to create disk name with empty string, and if user will send 0, it will be actually '0', but indeed better to be save. Thanks.

[gregdek]

Thanks again to @fabianvf for this PR, and thanks @machacekondra for reviewing. Marking for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

[machacekondra]

I would welcome backport to 2.2, if there will be another 2.2 release.

[resmo]

backported to 2.2 for 2.2.1