Merge pull request #2818 from mtnbikenc/package-refactor

Refactor to use Ansible package module

1 files changed, 15 insertions, 23 deletions

diff --git a/docs/best_practices_guide.adoc b/docs/best_practices_guide.adoc
index e9d904965..7f3d85d40 100644
--- a/docs/best_practices_guide.adoc
+++ b/docs/best_practices_guide.adoc
@@ -2,7 +2,7 @@
 
 = openshift-ansible Best Practices Guide
 
-The purpose of this guide is to describe the preferred patterns and best practices used in this repository (both in ansible and python).
+The purpose of this guide is to describe the preferred patterns and best practices used in this repository (both in Ansible and Python).
 
 It is important to note that this repository may not currently comply with all best practices, but the intention is that it will.
 
@@ -76,7 +76,7 @@ def add_person(first_name, last_name, age=None):
 
 
 === PyLint
-http://www.pylint.org/[PyLint] is used in an attempt to keep the python code as clean and as manageable as possible. The build bot runs each pull request through PyLint and any warnings or errors cause the build bot to fail the pull request.
+http://www.pylint.org/[PyLint] is used in an attempt to keep the Python code as clean and as manageable as possible. The build bot runs each pull request through PyLint and any warnings or errors cause the build bot to fail the pull request.
 
 '''
 [[PyLint-rules-MUST-NOT-be-disabled-on-a-whole-file]]
@@ -338,9 +338,9 @@ If an Ansible role requires certain variables to be set, it's best to check for
 [cols="2v,v"]
 |===
 | <<Ansible-tasks-SHOULD-NOT-be-used-in-ansible-playbooks-Instead-use-pre_tasks-and-post_tasks, Rule>>
-| Ansible tasks SHOULD NOT be used in ansible playbooks. Instead, use pre_tasks and post_tasks.
+| Ansible tasks SHOULD NOT be used in Ansible playbooks. Instead, use pre_tasks and post_tasks.
 |===
-An Ansible play is defined as a Yaml dictionary. Because of that, ansible doesn't know if the play's tasks list or roles list was specified first. Therefore Ansible always runs tasks after roles.
+An Ansible play is defined as a Yaml dictionary. Because of that, Ansible doesn't know if the play's tasks list or roles list was specified first. Therefore Ansible always runs tasks after roles.
 
 This can be quite confusing if the tasks list is defined in the playbook before the roles list because people assume in order execution in Ansible.
 
@@ -484,31 +484,23 @@ If you want to use default with variables that evaluate to false you have to set
 ----
 
 
-In other words, normally the `default` filter will only replace the value if it's undefined. By setting the second parameter to `true`, it will also replace the value if it defaults to a false value in python, so None, empty list, empty string, etc.
+In other words, normally the `default` filter will only replace the value if it's undefined. By setting the second parameter to `true`, it will also replace the value if it defaults to a false value in Python, so None, empty list, empty string, etc.
 
 This is almost always more desirable than an empty list, string, etc.
 
 === Yum and DNF
 '''
-[[Package-installation-MUST-use-ansible-action-module-to-abstract-away-dnf-yum]]
+[[Package-installation-MUST-use-ansible-package-module-to-abstract-away-dnf-yum]]
 [cols="2v,v"]
 |===
-| <<Package-installation-MUST-use-ansible-action-module-to-abstract-away-dnf-yum, Rule>>
-| Package installation MUST use ansible action module to abstract away dnf/yum.
+| <<Package-installation-MUST-use-ansible-package-module-to-abstract-away-dnf-yum, Rule>>
+| Package installation MUST use Ansible `package` module to abstract away dnf/yum.
 |===
 
-[[Package-installation-MUST-use-name-and-state-present-rather-than-pkg-and-state-installed-respectively]]
-[cols="2v,v"]
-|===
-| <<Package-installation-MUST-use-name-and-state-present-rather-than-pkg-and-state-installed-respectively, Rule>>
-| Package installation MUST use name= and state=present rather than pkg= and state=installed respectively.
-|===
+The Ansible `package` module calls the associated package manager for the underlying OS.
 
-This is done primarily because if you're registering the result of the
-installation and you have two conditional tasks based on whether or not yum or
-dnf are in use you'll end up inadvertently overwriting the value. It also
-reduces duplication. name= and state=present are common between dnf and yum
-modules.
+.Reference
+* https://docs.ansible.com/ansible/package_module.html[Ansible package module]
 
 .Bad:
 [source,yaml]
@@ -516,12 +508,12 @@ modules.
 ---
 # tasks.yml
 - name: Install etcd (for etcdctl)
-  yum: name=etcd state=latest"
+  yum: name=etcd state=latest
   when: "ansible_pkg_mgr == yum"
   register: install_result
 
 - name: Install etcd (for etcdctl)
-  dnf: name=etcd state=latest"
+  dnf: name=etcd state=latest
   when: "ansible_pkg_mgr == dnf"
   register: install_result
 ----
@@ -533,6 +525,6 @@ modules.
 ---
 # tasks.yml
 - name: Install etcd (for etcdctl)
-  action: "{{ ansible_pkg_mgr }} name=etcd state=latest"
+  package: name=etcd state=latest
   register: install_result
-  ----
+----