diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/best_practices_guide.adoc | 159 | ||||
| -rw-r--r-- | docs/style_guide.adoc | 22 |
2 files changed, 145 insertions, 36 deletions
diff --git a/docs/best_practices_guide.adoc b/docs/best_practices_guide.adoc index 6aaf5228a..e9d904965 100644 --- a/docs/best_practices_guide.adoc +++ b/docs/best_practices_guide.adoc @@ -13,9 +13,12 @@ This guide complies with https://www.ietf.org/rfc/rfc2119.txt[RFC2119]. == Pull Requests + + +[[All-pull-requests-MUST-pass-the-build-bot-before-they-are-merged]] [cols="2v,v"] |=== -| **Rule** +| <<All-pull-requests-MUST-pass-the-build-bot-before-they-are-merged, Rule>> | All pull requests MUST pass the build bot *before* they are merged. |=== @@ -30,9 +33,10 @@ The tooling is flexible enough that exceptions can be made so that the tool the === Python Source Files ''' +[[Python-source-files-MUST-contain-the-following-vim-mode-line]] [cols="2v,v"] |=== -| **Rule** +| <<Python-source-files-MUST-contain-the-following-vim-mode-line, Rule>> | Python source files MUST contain the following vim mode line. |=== @@ -48,10 +52,11 @@ If mode lines for other editors are needed, please open a GitHub issue. === Method Signatures ''' +[[When-adding-a-new-parameter-to-an-existing-method-a-default-value-SHOULD-be-used]] [cols="2v,v"] |=== -| **Rule** -| When adding a new paramemter to an existing method, a default value SHOULD be used +| <<When-adding-a-new-parameter-to-an-existing-method-a-default-value-SHOULD-be-used, Rule>> +| When adding a new parameter to an existing method, a default value SHOULD be used |=== The purpose of this rule is to make it so that method signatures are backwards compatible. @@ -71,21 +76,23 @@ 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 managable 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]] [cols="2v,v"] |=== -| **Rule** +| <<PyLint-rules-MUST-NOT-be-disabled-on-a-whole-file, Rule>> | PyLint rules MUST NOT be disabled on a whole file. |=== Instead, http://docs.pylint.org/faq.html#is-it-possible-to-locally-disable-a-particular-message[disable the PyLint check on the line where PyLint is complaining]. ''' +[[PyLint-rules-MUST-NOT-be-disabled-unless-they-meet-one-of-the-following-exceptions]] [cols="2v,v"] |=== -| **Rule** +| <<PyLint-rules-MUST-NOT-be-disabled-unless-they-meet-one-of-the-following-exceptions, Rule>> | PyLint rules MUST NOT be disabled unless they meet one of the following exceptions |=== @@ -95,9 +102,10 @@ Instead, http://docs.pylint.org/faq.html#is-it-possible-to-locally-disable-a-par 1. When PyLint fails, but the code makes more sense the way it is formatted (stylistic exception). For this exception, the description of the PyLint disable MUST state why the code is more clear, AND the person reviewing the PR will decide if they agree or not. The reviewer may reject the PR if they disagree with the reason for the disable. ''' +[[All-PyLint-rule-disables-MUST-be-documented-in-the-code]] [cols="2v,v"] |=== -| **Rule** +| <<All-PyLint-rule-disables-MUST-be-documented-in-the-code, Rule>> | All PyLint rule disables MUST be documented in the code. |=== @@ -124,9 +132,10 @@ metadata[line] = results.pop() === Yaml Files (Playbooks, Roles, Vars, etc) ''' +[[Ansible-files-SHOULD-NOT-use-JSON-use-pure-YAML-instead]] [cols="2v,v"] |=== -| **Rule** +| <<Ansible-files-SHOULD-NOT-use-JSON-use-pure-YAML-instead, Rule>> | Ansible files SHOULD NOT use JSON (use pure YAML instead). |=== @@ -144,13 +153,48 @@ Every effort should be made to keep our Ansible YAML files in pure YAML. === Modules ''' +[[Custom-Ansible-modules-SHOULD-be-embedded-in-a-role]] +[cols="2v,v"] +|=== +| <<Custom-Ansible-modules-SHOULD-be-embedded-in-a-role, Rule>> +| Custom Ansible modules SHOULD be embedded in a role. +|=== + +.Context +* http://docs.ansible.com/ansible/playbooks_roles.html#embedding-modules-in-roles[Ansible doc on how to embed modules in roles] + +The purpose of this rule is to make it easy to include custom modules in our playbooks and share them on Ansible Galaxy. + +.Custom module `openshift_facts.py` is embedded in the `openshift_facts` role. +---- +> ll openshift-ansible/roles/openshift_facts/library/ +-rwxrwxr-x. 1 user group 33616 Jul 22 09:36 openshift_facts.py +---- + +.Custom module `openshift_facts` can be used after `openshift_facts` role has been referenced. +[source,yaml] +---- +- hosts: openshift_hosts + gather_facts: no + roles: + - role: openshift_facts + post_tasks: + - openshift_facts + role: common + hostname: host + public_hostname: host.example.com +---- + + +''' +[[Parameters-to-Ansible-modules-SHOULD-use-the-Yaml-dictionary-format-when-3-or-more-parameters-are-being-passed]] [cols="2v,v"] |=== -| **Rule** -| Parameters to Ansible Modules SHOULD use the Yaml dictionary format when 3 or more parameters are being passed +| <<Parameters-to-Ansible-modules-SHOULD-use-the-Yaml-dictionary-format-when-3-or-more-parameters-are-being-passed, Rule>> +| Parameters to Ansible modules SHOULD use the Yaml dictionary format when 3 or more parameters are being passed |=== -When a module has several parameters that are being passed in, it's hard to see exactly what value each parameter is getting. It is preferred to use the Ansible Yaml syntax to pass in parameters so that it's more clear what values are being passed for each paramemter. +When a module has several parameters that are being passed in, it's hard to see exactly what value each parameter is getting. It is preferred to use the Ansible Yaml syntax to pass in parameters so that it's more clear what values are being passed for each parameter. .Bad: [source,yaml] @@ -171,13 +215,14 @@ When a module has several parameters that are being passed in, it's hard to see ''' +[[Parameters-to-Ansible-modules-SHOULD-use-the-Yaml-dictionary-format-when-the-line-length-exceeds-120-characters]] [cols="2v,v"] |=== -| **Rule** -| Parameters to Ansible Modules SHOULD use the Yaml dictionary format when the line length exceeds 120 characters +| <<Parameters-to-Ansible-modules-SHOULD-use-the-Yaml-dictionary-format-when-the-line-length-exceeds-120-characters, Rule>> +| Parameters to Ansible modules SHOULD use the Yaml dictionary format when the line length exceeds 120 characters |=== -Lines that are long quickly become a wall of text that isn't easily parsable. It is preferred to use the Ansible Yaml syntax to pass in parameters so that it's more clear what values are being passed for each paramemter. +Lines that are long quickly become a wall of text that isn't easily parsable. It is preferred to use the Ansible Yaml syntax to pass in parameters so that it's more clear what values are being passed for each parameter. .Bad: [source,yaml] @@ -195,13 +240,14 @@ Lines that are long quickly become a wall of text that isn't easily parsable. It ---- ''' +[[The-Ansible-command-module-SHOULD-be-used-instead-of-the-Ansible-shell-module]] [cols="2v,v"] |=== -| **Rule** -| The Ansible `shell` module SHOULD NOT be used. Instead, use the `command` module. +| <<The-Ansible-command-module-SHOULD-be-used-instead-of-the-Ansible-shell-module, Rule>> +| The Ansible `command` module SHOULD be used instead of the Ansible `shell` module. |=== .Context -* http://docs.ansible.com/shell_module.html#notes[Ansible Doc on why using the command module is a best practice] +* http://docs.ansible.com/shell_module.html#notes[Ansible doc on why using the command module is a best practice] The Ansible `shell` module can run most commands that can be run from a bash CLI. This makes it extremely powerful, but it also opens our playbooks up to being exploited by attackers. @@ -218,13 +264,14 @@ The Ansible `shell` module can run most commands that can be run from a bash CLI ---- ''' +[[The-Ansible-quote-filter-MUST-be-used-with-any-variable-passed-into-the-shell-module]] [cols="2v,v"] |=== -| **Rule** +| <<The-Ansible-quote-filter-MUST-be-used-with-any-variable-passed-into-the-shell-module, Rule>> | The Ansible `quote` filter MUST be used with any variable passed into the shell module. |=== .Context -* http://docs.ansible.com/shell_module.html#notes[Ansible Doc describing why to use the quote filter] +* http://docs.ansible.com/shell_module.html#notes[Ansible doc describing why to use the quote filter] It is recommended not to use the `shell` module. However, if it absolutely must be used, all variables passed into the `shell` module MUST use the `quote` filter to ensure they are shell safe. @@ -246,9 +293,10 @@ It is recommended not to use the `shell` module. However, if it absolutely must * http://docs.ansible.com/fail_module.html[Ansible Fail Module] ''' +[[Ansible-playbooks-MUST-begin-with-checks-for-any-variables-that-they-require]] [cols="2v,v"] |=== -| **Rule** +| <<Ansible-playbooks-MUST-begin-with-checks-for-any-variables-that-they-require, Rule>> | Ansible playbooks MUST begin with checks for any variables that they require. |=== @@ -266,9 +314,10 @@ If an Ansible playbook requires certain variables to be set, it's best to check ---- ''' +[[Ansible-roles-tasks-main-yml-file-MUST-begin-with-checks-for-any-variables-that-they-require]] [cols="2v,v"] |=== -| **Rule** +| <<Ansible-roles-tasks-main-yml-file-MUST-begin-with-checks-for-any-variables-that-they-require, Rule>> | Ansible roles tasks/main.yml file MUST begin with checks for any variables that they require. |=== @@ -285,9 +334,10 @@ If an Ansible role requires certain variables to be set, it's best to check for === Tasks ''' +[[Ansible-tasks-SHOULD-NOT-be-used-in-ansible-playbooks-Instead-use-pre_tasks-and-post_tasks]] [cols="2v,v"] |=== -| **Rule** +| <<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. |=== 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. @@ -337,9 +387,10 @@ Therefore, we SHOULD use pre_tasks and post_tasks to make it more clear when the === Roles ''' +[[All-tasks-in-a-role-SHOULD-be-tagged-with-the-role-name]] [cols="2v,v"] |=== -| **Rule** +| <<All-tasks-in-a-role-SHOULD-be-tagged-with-the-role-name, Rule>> | All tasks in a role SHOULD be tagged with the role name. |=== @@ -362,9 +413,10 @@ This is very useful when developing and debugging new tasks. It can also signifi ''' +[[The-Ansible-roles-directory-MUST-maintain-a-flat-structure]] [cols="2v,v"] |=== -| **Rule** +| <<The-Ansible-roles-directory-MUST-maintain-a-flat-structure, Rule>> | The Ansible roles directory MUST maintain a flat structure. |=== @@ -377,9 +429,10 @@ This is very useful when developing and debugging new tasks. It can also signifi * Make it compatible with Ansible Galaxy ''' +[[Ansible-Roles-SHOULD-be-named-like-technology_component_subcomponent]] [cols="2v,v"] |=== -| **Rule** +| <<Ansible-Roles-SHOULD-be-named-like-technology_component_subcomponent, Rule>> | Ansible Roles SHOULD be named like technology_component[_subcomponent]. |=== @@ -388,7 +441,7 @@ For consistency, role names SHOULD follow the above naming pattern. It is import Many times the `technology` portion of the pattern will line up with a package name. It is advised that whenever possible, the package name should be used. .Examples: -* The role to configure an OpenShift Master is called `openshift_master` +* The role to configure a master is called `openshift_master` * The role to configure OpenShift specific yum repositories is called `openshift_repos` === Filters @@ -397,9 +450,10 @@ Many times the `technology` portion of the pattern will line up with a package n * http://jinja.pocoo.org/docs/dev/templates/#builtin-filters[Jinja2 Builtin Filters] ''' +[[The-default-filter-SHOULD-replace-empty-strings-lists-etc]] [cols="2v,v"] |=== -| **Rule** +| <<The-default-filter-SHOULD-replace-empty-strings-lists-etc, Rule>> | The `default` filter SHOULD replace empty strings, lists, etc. |=== @@ -433,3 +487,52 @@ 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. 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]] +[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-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. +|=== + +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. + +.Bad: +[source,yaml] +---- +--- +# tasks.yml +- name: Install etcd (for etcdctl) + yum: name=etcd state=latest" + when: "ansible_pkg_mgr == yum" + register: install_result + +- name: Install etcd (for etcdctl) + dnf: name=etcd state=latest" + when: "ansible_pkg_mgr == dnf" + register: install_result +---- + + +.Good: +[source,yaml] +---- +--- +# tasks.yml +- name: Install etcd (for etcdctl) + action: "{{ ansible_pkg_mgr }} name=etcd state=latest" + register: install_result + ---- diff --git a/docs/style_guide.adoc b/docs/style_guide.adoc index 110e7153f..2c2cb8610 100644 --- a/docs/style_guide.adoc +++ b/docs/style_guide.adoc @@ -19,9 +19,10 @@ This style guide complies with https://www.ietf.org/rfc/rfc2119.txt[RFC2119]. * https://www.python.org/dev/peps/pep-0008/#maximum-line-length[Python Pep8 Line Length] ''' +[[All-lines-SHOULD-be-no-longer-than-80-characters]] [cols="2v,v"] |=== -| **Rule** +| <<All-lines-SHOULD-be-no-longer-than-80-characters, Rule>> | All lines SHOULD be no longer than 80 characters. |=== @@ -31,9 +32,10 @@ Code readability is subjective, therefore pull-requests SHOULD still be merged, ''' +[[All-lines-MUST-be-no-longer-than-120-characters]] [cols="2v,v"] |=== -| **Rule** +| <<All-lines-MUST-be-no-longer-than-120-characters, Rule>> | All lines MUST be no longer than 120 characters. |=== @@ -46,9 +48,10 @@ This is a hard limit and is enforced by the build bot. This check MUST NOT be di === Ansible Yaml file extension ''' +[[All-Ansible-Yaml-files-MUST-have-a-yml-extension-and-NOT-YML-yaml-etc]] [cols="2v,v"] |=== -| **Rule** +| <<All-Ansible-Yaml-files-MUST-have-a-yml-extension-and-NOT-YML-yaml-etc, Rule>> | All Ansible Yaml files MUST have a .yml extension (and NOT .YML, .yaml etc). |=== @@ -59,13 +62,14 @@ Example: `tasks.yml` === Ansible CLI Variables ''' +[[Variables-meant-to-be-passed-in-from-the-ansible-CLI-MUST-have-a-prefix-of-cli]] [cols="2v,v"] |=== -| **Rule** +| <<Variables-meant-to-be-passed-in-from-the-ansible-CLI-MUST-have-a-prefix-of-cli, Rule>> | Variables meant to be passed in from the ansible CLI MUST have a prefix of cli_ |=== -Ansible allows variables to be passed in on the command line using the `-e` option. These variables MUST have a prefix of cli_ so that it's clear where they came from, and how dangerous they are (can be exploited). +Ansible allows variables to be passed in on the command line using the `-e` option. These variables MUST have a prefix of cli_ so that it's clear where they came from, and that they could be exploited. .Example: @@ -76,9 +80,10 @@ ansible-playbook -e cli_foo=bar someplays.yml === Ansible Global Variables ''' +[[Global-variables-MUST-have-a-prefix-of-g]] [cols="2v,v"] |=== -| **Rule** +| <<Global-variables-MUST-have-a-prefix-of-g, Rule>> | Global variables MUST have a prefix of g_ |=== Ansible global variables are defined as any variables outside of ansible roles. Examples include playbook variables, variables passed in on the cli, etc. @@ -94,10 +99,11 @@ g_environment: someval Ansible role variables are defined as variables contained in (or passed into) a role. ''' +[[Role-variables-MUST-have-a-prefix-of-atleast-3-characters-See.below.for.specific.naming.rules]] [cols="2v,v"] |=== -| **Rule** -| Role variables MUST have a prefix of atleast 3 characters. See below for specific naming rules. +| <<Role-variables-MUST-have-a-prefix-of-atleast-3-characters-See.below.for.specific.naming.rules, Rule>> +| Role variables MUST have a prefix of at least 3 characters. See below for specific naming rules. |=== ==== Role with 3 (or more) words in the name |