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

vmware_guest: Fix disk parameter validation #705

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

mariolenz
Copy link
Collaborator

SUMMARY

The module fails when disk.controller_number or disk.unit_number are 0.

Fixes #703

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

vmware_guest

ADDITIONAL INFORMATION

if not disk_spec['controller_type'] or not disk_spec['controller_number'] or not disk_spec['unit_number']:

When disk.controller_number or disk.unit_number are 0, the module fails because not 0 is True. 0 is a valid value, the module should only fail if these parameters are not defined at all.

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug cloud community_review has_issue module module needs_triage Needs a first human triage before being processed. plugins plugin (any type) small_patch Hopefully easy to review labels Mar 9, 2021
@@ -2477,7 +2477,7 @@ def sanitize_disk_parameters(self, vm_obj):
"""
controllers = []
for disk_spec in self.params.get('disk'):
if not disk_spec['controller_type'] or not disk_spec['controller_number'] or not disk_spec['unit_number']:
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add an integration test for this change? REST LGTM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Akasurde I think there already exists a test case for controller_number / unit_number 0:

- controller_type: lsilogicsas
controller_number: 0
unit_number: 0
size_mb: 512
type: thin
- controller_type: paravirtual
controller_number: 1
unit_number: 0
size_mb: 256
type: eagerzeroedthick

But I don't understand why this didn't fail in the CI :-/

Copy link
Member

Choose a reason for hiding this comment

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

@ansibullbot ansibullbot added integration tests/integration tests tests labels Mar 9, 2021
@@ -19,6 +19,7 @@ vmware_guest_test_playbooks:
- mac_address_d1_c1_f0.yml
- max_connections.yml
- mem_reservation.yml
- multiple_disk_controllers_d1_c1_f0.yml
Copy link
Member

Choose a reason for hiding this comment

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

+1, hopefully it won't blow things up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@goneri Unfortunately, it did blow things up. I'll try to find out why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Akasurde @goneri I think this is yet another problem with the vCenter simulator. I have moved those tests into a when: vcsim is not defined block.

Btw: It looks like we don't test against a real vCenter in the CI pipeline anymore. Would it be possible to get this back? It's really unfortunate that there are so many integration tests that are skipped because we only have vcsim.

@ansibullbot ansibullbot removed the small_patch Hopefully easy to review label Mar 10, 2021
@mariolenz
Copy link
Collaborator Author

@Akasurde @goneri What do you think, should we merge this PR? According to @MalfuncEddie it works and fixes the bug.

@Akasurde Akasurde added gate and removed needs_triage Needs a first human triage before being processed. labels Mar 15, 2021
@Akasurde Akasurde added this to the v1.9.0 milestone Mar 15, 2021
@mariolenz
Copy link
Collaborator Author

@Akasurde @goneri Did something change in the CI? ansible-test-sanity-docker worked in ansible/check but failed in ansible/gate.

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 cloud community_review has_issue integration tests/integration module module plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disk.controller_number and disk.unit_number set to 0 results in validation error because 0 == false in v1.8
4 participants