Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Add ovirt_datacenters and ovirt_datacenters_facts modules #3146

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Add ovirt_datacenters and ovirt_datacenters_facts modules #3146

merged 1 commit into from
Dec 5, 2016

Conversation

machacekondra
Copy link
Contributor

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
  • ovirt_datacenters
  • ovirt_datacenters_facts
ANSIBLE VERSION
ansible 2.1.1.0
SUMMARY

Add ovirt_datacenters and ovirt_datacenters_facts modules to manage oVirt datacenters.

@machacekondra machacekondra force-pushed the ovirt_datacenters branch 3 times, most recently from 2378cb1 to 1c1feaf Compare October 12, 2016 13:09
@machacekondra
Copy link
Contributor Author

@mwperina @ryansb

@machacekondra
Copy link
Contributor Author

ready_for_review

@gregdek
Copy link
Contributor

gregdek commented Oct 12, 2016

Thanks @machacekondra for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

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

machacekondra added a commit to machacekondra/ovirt-tests that referenced this pull request Oct 13, 2016
@machacekondra
clusters, dcs, networks PR ready
408060d 
ansible/ansible-modules-extras#3148

ansible/ansible-modules-extras#3146

ansible/ansible-modules-extras#3138
@machacekondra machacekondra force-pushed the ovirt_datacenters branch 2 times, most recently from 0877b58 to 440223f Compare October 14, 2016 09:57
@machacekondra
Copy link
Contributor Author

shipit

@gregdek
Copy link
Contributor

gregdek commented Oct 14, 2016

Thanks @machacekondra. Since you are one of the maintainers (@machacekondra) of this module and you commented with "shipit", we are marking this PR for inclusion.

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

mwperina
mwperina suggested changes Oct 19, 2016
View reviewed changes
cloud/ovirt/ovirt_datacenters.py
- ovirt_datacenters:
name: mydatacenter
local: True
compatibility_version: 4.1
Copy link

Choose a reason for hiding this comment

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

Please use version 4.0, as 4.1 may not be released before Ansible 2.3

mwperina
mwperina reviewed Oct 19, 2016
View reviewed changes
cloud/ovirt/ovirt_datacenters_facts.py
# Examples don't contain auth parameter for simplicity,
# look at ovirt_auth module to see how to reuse authentication:

# Gather facts about all datacenters named C<production*>:
Copy link

@mwperina mwperina Oct 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rephrase to: 'Gather facts about all data centers which names start with C:'

mwperina
mwperina suggested changes Oct 19, 2016
View reviewed changes
Copy link

@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.

Generally looks good, just 2 small doc issues

@machacekondra machacekondra force-pushed the ovirt_datacenters branch 2 times, most recently from 898be08 to 57c7217 Compare October 19, 2016 12:28
mwperina
mwperina suggested changes Oct 20, 2016
View reviewed changes
cloud/ovirt/ovirt_datacenters_facts.py
# Examples don't contain auth parameter for simplicity,
# look at ovirt_auth module to see how to reuse authentication:

# Gather facts about all data centers which names start with C<production*>:
Copy link

Choose a reason for hiding this comment

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

'C(production*)' -> 'C(production)'

mwperina
mwperina suggested changes Oct 20, 2016
View reviewed changes
cloud/ovirt/ovirt_datacenters_facts.py
# Examples don't contain auth parameter for simplicity,
# look at ovirt_auth module to see how to reuse authentication:

# Gather facts about all data centers which names start with C<production>:
Copy link

Choose a reason for hiding this comment

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

C<production> -> C(production)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mwperina
mwperina approved these changes Oct 21, 2016
View reviewed changes
Copy link

@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.

Looks good to me, thanks!

@bcoca bcoca removed the shipit label Nov 8, 2016
@gregdek
Copy link
Contributor

gregdek commented Nov 8, 2016

Thanks again to @machacekondra. This module is going into community review.

We notice that you are one of the maintainers (@machacekondra) of this module, so when you think it's ready, please comment "shipit" and we will consider it for merging.

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

ryansb
ryansb reviewed Nov 8, 2016
View reviewed changes
cloud/ovirt/ovirt_datacenters.py

try:
import ovirtsdk4 as sdk
import ovirtsdk4.types as otypes
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a conditional on these (similar to the AWS modules) so your module doesn't raise ImportError at load.

@ryansb
Copy link
Contributor

ryansb commented Nov 8, 2016

Looks good overall, just add a failure mode for when the oVirt SDK isn't installed.

@machacekondra
Copy link
Contributor Author

shipit

@gregdek
Copy link
Contributor

gregdek commented Nov 22, 2016

Thanks @machacekondra. Since you are one of the maintainers (@machacekondra) of this module and you commented with "shipit", we are marking this PR for inclusion.

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

@gregdek gregdek added the shipit label Nov 22, 2016
mwperina
mwperina approved these changes Nov 24, 2016
View reviewed changes
Copy link

@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.

👍

@ryansb ryansb merged commit 4354a5d into ansible:devel Dec 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryansb ryansb ryansb left review comments

@mwperina mwperina mwperina approved these changes

Assignees
No one assigned
Labels
cloud new_plugin owner_pr shipit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@machacekondra @gregdek @ryansb @mwperina @bcoca