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

ovrit_cluster: fix for CPU arch entity #37425 #37436

Merged
merged 3 commits into from Mar 21, 2018
Merged

ovrit_cluster: fix for CPU arch entity #37425 #37436

merged 3 commits into from Mar 21, 2018

Conversation

mkrish004c
Copy link

SUMMARY

CPU arch entity is passing the value as it is from the request, this would validate it.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ovirt_cluster

ANSIBLE VERSION
Ansible 2.4.0.0
ADDITIONAL INFORMATION

This fix would take care of below error message, when the request comes with CPU arch defined.

- name: set cluster policies
  ovirt_cluster:
    auth: "{{ ovirt_auth }}"
    name: "{{ rhv_cluster_name }}"
    data_center: "{{ rhv_datacenter_name }}"
    cpu_arch: x86_64
    cpu_type: Intel Broadwell-noTSX Family

The error,

"msg": "The type 'str' isn't valid for attribute 'architecture', it must be 'Architecture'"

@ansibot
Copy link
Contributor

ansibot commented Mar 14, 2018

cc @machacekondra @mwperina
click here for bot help

@ansibot ansibot added bug This issue/PR relates to a bug. cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. virt Virt community (incl. QEMU, KVM, libvirt, ovirt, RHV and Proxmox) support:community This issue/PR relates to code supported by the Ansible community. labels Mar 14, 2018
@ansibot
Copy link
Contributor

ansibot commented Mar 14, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/ovirt/ovirt_cluster.py:489:22: E126 continuation line over-indented for hanging indent

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 14, 2018
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 14, 2018
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Mar 15, 2018
machacekondra
machacekondra approved these changes Mar 15, 2018
View reviewed changes
Copy link
Contributor

@machacekondra machacekondra left a comment

Choose a reason for hiding this comment

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

Thanks!

@machacekondra
Copy link
Contributor

shipit

machacekondra
machacekondra reviewed Mar 15, 2018
View reviewed changes
lib/ansible/modules/cloud/ovirt/ovirt_cluster.py Outdated
architecture=self.param('cpu_arch'),
architecture=otypes.Architecture(
self.param('cpu_arch')
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, can you change it to:

achitecture=otypes.Architecture(
  self.param('cpu_arch')
) if self.param('cpu_arch') else None,

So if cpu_arch parameter isn't passed we don't try to sent empty architecture parameter.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, i guess that case would be handled by this fix, which is in place already.

#34446

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, that's different case. The code above I've sent:

achitecture=otypes.Architecture(
  self.param('cpu_arch')
) if self.param('cpu_arch') else None,

Ensures it sent <achitecture/> xml element when creating/updating cluster only when user passes cpu_arch parameter to Ansible. The current code you've sent will sent <achitecture/> element also when user don't cpu_arch parameter.

Copy link
Author

Choose a reason for hiding this comment

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

Cool got it, Thanks! changed it.

@machacekondra
Copy link
Contributor

fixes: #37425

machacekondra
machacekondra approved these changes Mar 15, 2018
View reviewed changes
Copy link
Contributor

@machacekondra machacekondra left a comment

Choose a reason for hiding this comment

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

Thanks! shipit

mwperina
mwperina approved these changes Mar 21, 2018
View reviewed changes
Copy link
Contributor

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

shipit

@ansibot ansibot added automerge This PR was automatically merged by ansibot. shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 21, 2018
@ansibot ansibot merged commit 7a4c3e5 into ansible:devel Mar 21, 2018
@mkrish004c mkrish004c deleted the patch-1 branch March 21, 2018 13:59
machacekondra pushed a commit to machacekondra/ansible that referenced this pull request Apr 5, 2018
@machacekondra
ovrit_cluster: fix for CPU arch entity ansible#37425 (ansible#37436)
3f4653f 
* ovrit_cluster: fix for CPU arch entity ansible#37425

* Corrected Indentation

* Condition to check if `architecture` is defined
@machacekondra machacekondra mentioned this pull request Apr 5, 2018
Merged
nitzmahone pushed a commit that referenced this pull request Apr 9, 2018
@machacekondra @nitzmahone
ovirt: Cherry picks of recent fixes to devel branch (#38341)
9d0ebfd 
* ovirt_host_networks: Fix idempotency

* ovirt_hosts: Fix failed_state after PM fence

* ovirt_host_networks: Fix of bond assignment (#38054)

* ovirt_host_networks: Fix of bond assignment

* ovirt_host_networks: Fix bond modes

* ovirt_host_networks: Fix incorrect prefix documentation

* ovrit_cluster: fix for CPU arch entity #37425 (#37436)

* ovrit_cluster: fix for CPU arch entity #37425

* Corrected Indentation

* Condition to check if `architecture` is defined
@dagwieers dagwieers added ovirt oVirt and RHV community and removed virt Virt community (incl. QEMU, KVM, libvirt, ovirt, RHV and Proxmox) labels Feb 21, 2019
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@machacekondra machacekondra machacekondra approved these changes

@mwperina mwperina mwperina approved these changes

Assignees
No one assigned
Labels
automerge This PR was automatically merged by ansibot. bug This issue/PR relates to a bug. cloud module This issue/PR relates to a module. new_contributor This PR is the first contribution by a new community member. ovirt oVirt and RHV community shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mkrish004c @ansibot @machacekondra @mwperina @dagwieers @jborean93