From 437a88e4516d8c99211cbce535a25532bd3c5493 Mon Sep 17 00:00:00 2001 From: Thomas Wiest Date: Tue, 26 May 2015 10:15:31 -0400 Subject: Added concepts guide. --- docs/best_practices_guide.adoc | 122 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 docs/best_practices_guide.adoc (limited to 'docs/best_practices_guide.adoc') diff --git a/docs/best_practices_guide.adoc b/docs/best_practices_guide.adoc new file mode 100644 index 000000000..938b6b46a --- /dev/null +++ b/docs/best_practices_guide.adoc @@ -0,0 +1,122 @@ +// vim: ft=asciidoc + += 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). + +It is important to note that this repository may not currently comply with all best practices, but our intention is that it will. + +All new pull requests created against this repository MUST comply with this guide. + +This guide complies with https://www.ietf.org/rfc/rfc2119.txt[RFC2119]. + + +== Pull Requests + +[cols="2v,v"] +|=== +| **Rule** +| All pull requests MUST pass the build bot *before* they are merged. +|=== + + +The purpose of this rule is to avoid cases where the build bot will fail pull requests for code modified in a previous pull request. + +Our tooling is flexible enough that exceptions can be made so that the tool the build bot is running will ignore certain areas or certain checks, but the build bot itself must pass for the pull request to be merged. + + + +== Python + +=== PyLint +We use http://www.pylint.org/[PyLint] in an attempt to keep our python code as clean and as managable as possible. Our build bot runs each pull request through PyLint and any warnings or errors cause the build bot to fail the pull request. + +''' +[cols="2v,v"] +|=== +| **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]. + +''' +[cols="2v,v"] +|=== +| **Rule** +| PyLint rules MUST NOT be disabled unless they meet one of the following exceptions +|=== + +.Exceptions: +1. When PyLint fails because of a dependency that can't be installed on the build bot +1. When PyLint fails because we are including a module that is outside of our control (like Ansible) + +''' +[cols="2v,v"] +|=== +| **Rule** +| All PyLint rule disables MUST be documented in the code. +|=== + +The purpose of this rule is to inform future developers about the disable. + +.Specifically, the following MUST accompany every PyLint disable: +1. Why is the check being disabled? +1. Is disabling this check meant to be permanent or temporary? + +.Example: +[source,python] +---- +# Reason: disable pylint maybe-no-member because overloaded use of +# the module name causes pylint to not detect that 'results' +# is an array or hash +# Status: permanently disabled unless we can find a way to fix this. +# pylint: disable=maybe-no-member +metadata[line] = results.pop() +---- + + +== Ansible + +=== Filters +.Context: +* https://docs.ansible.com/playbooks_filters.html[Ansible Playbook Filters] +* http://jinja.pocoo.org/docs/dev/templates/#builtin-filters[Jinja2 Builtin Filters] + +''' +[cols="2v,v"] +|=== +| **Rule** +| The `default` filter SHOULD replace empty strings, lists, etc. +|=== + +When using the jinja2 `default` filter, unless the variable is a boolean, specify `true` as the second parameter. This will cause the default filter to replace empty strings, lists, etc with the provided default. + +This is because we would prefer to either have a sane default set than to have an empty string, list, etc. We don't, for example, want config values set to an empty string. + +.From the http://jinja.pocoo.org/docs/dev/templates/[Jinja2 Docs]: +[quote] +If you want to use default with variables that evaluate to false you have to set the second parameter to true + +.Example: +[source,yaml] +---- +--- +- hosts: localhost + gather_facts: no + vars: + somevar: '' + tasks: + - debug: var=somevar + + - name: "Will output 'somevar: []'" + debug: "msg='somevar: [{{ somevar | default('the string was empty') }}]'" + + - name: "Will output 'somevar: [the string was empty]'" + debug: "msg='somevar: [{{ somevar | default('the string was empty', true) }}]'" +---- + + +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. + +We almost always want this instead of the empty list, string, etc. -- cgit v1.2.3 From 1e1de9761184eb0732d4880bfb4188097530249f Mon Sep 17 00:00:00 2001 From: Thomas Wiest Date: Tue, 26 May 2015 16:40:46 -0400 Subject: removed references to 'we' --- docs/best_practices_guide.adoc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'docs/best_practices_guide.adoc') diff --git a/docs/best_practices_guide.adoc b/docs/best_practices_guide.adoc index 938b6b46a..a839cd203 100644 --- a/docs/best_practices_guide.adoc +++ b/docs/best_practices_guide.adoc @@ -29,7 +29,7 @@ Our tooling is flexible enough that exceptions can be made so that the tool the == Python === PyLint -We use http://www.pylint.org/[PyLint] in an attempt to keep our python code as clean and as managable as possible. Our 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 our python code as clean and as managable as possible. Our build bot runs each pull request through PyLint and any warnings or errors cause the build bot to fail the pull request. ''' [cols="2v,v"] @@ -49,7 +49,7 @@ Instead, http://docs.pylint.org/faq.html#is-it-possible-to-locally-disable-a-par .Exceptions: 1. When PyLint fails because of a dependency that can't be installed on the build bot -1. When PyLint fails because we are including a module that is outside of our control (like Ansible) +1. When PyLint fails because of including a module that is outside of our control (like Ansible) ''' [cols="2v,v"] @@ -70,7 +70,7 @@ The purpose of this rule is to inform future developers about the disable. # Reason: disable pylint maybe-no-member because overloaded use of # the module name causes pylint to not detect that 'results' # is an array or hash -# Status: permanently disabled unless we can find a way to fix this. +# Status: permanently disabled unless a way is found to fix this. # pylint: disable=maybe-no-member metadata[line] = results.pop() ---- @@ -92,7 +92,7 @@ metadata[line] = results.pop() When using the jinja2 `default` filter, unless the variable is a boolean, specify `true` as the second parameter. This will cause the default filter to replace empty strings, lists, etc with the provided default. -This is because we would prefer to either have a sane default set than to have an empty string, list, etc. We don't, for example, want config values set to an empty string. +This is because it is preferable to either have a sane default set than to have an empty string, list, etc. For example, it is preferable to have a config value set to a sane default than to have it simply set as an empty string. .From the http://jinja.pocoo.org/docs/dev/templates/[Jinja2 Docs]: [quote] @@ -119,4 +119,4 @@ 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. -We almost always want this instead of the empty list, string, etc. +This is almost always more desirable than an empty list, string, etc. -- cgit v1.2.3 From 2df865bb6d627f991fd69b6c554c52f15bdcdf62 Mon Sep 17 00:00:00 2001 From: Thomas Wiest Date: Wed, 27 May 2015 16:00:16 -0400 Subject: added section for role naming to the best practices guide. --- docs/best_practices_guide.adoc | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) (limited to 'docs/best_practices_guide.adoc') diff --git a/docs/best_practices_guide.adoc b/docs/best_practices_guide.adoc index 83df53735..301c6ccda 100644 --- a/docs/best_practices_guide.adoc +++ b/docs/best_practices_guide.adoc @@ -78,7 +78,7 @@ metadata[line] = results.pop() == Ansible -=== Role Directory +=== Roles .Context * http://docs.ansible.com/playbooks_best_practices.html#directory-layout[Ansible Suggested Directory Layout] @@ -94,6 +94,20 @@ metadata[line] = results.pop() * Make it familiar for new contributors * Make it compatible with Ansible Galaxy +''' +[cols="2v,v"] +|=== +| **Rule** +| Ansible Roles SHOULD be named like technology_component[_subcomponent]. +|=== + +For clarity, it is suggested to follow a pattern when naming roles. It is important to note that this is a recommendation for role naming, and follows the pattern used by upstream. + +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 OpenShift specific yum repositories is called `openshift_repos` === Filters .Context: -- cgit v1.2.3 From 768c91b58822b41b44203c930d10a6fdc5058674 Mon Sep 17 00:00:00 2001 From: Thomas Wiest Date: Wed, 3 Jun 2015 14:38:10 -0400 Subject: Added 'stylistic exception' to the best practices guide. --- docs/best_practices_guide.adoc | 1 + 1 file changed, 1 insertion(+) (limited to 'docs/best_practices_guide.adoc') diff --git a/docs/best_practices_guide.adoc b/docs/best_practices_guide.adoc index 301c6ccda..af1acd94f 100644 --- a/docs/best_practices_guide.adoc +++ b/docs/best_practices_guide.adoc @@ -50,6 +50,7 @@ Instead, http://docs.pylint.org/faq.html#is-it-possible-to-locally-disable-a-par .Exceptions: 1. When PyLint fails because of a dependency that can't be installed on the build bot 1. When PyLint fails because of including a module that is outside of control (like Ansible) +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. ''' [cols="2v,v"] -- cgit v1.2.3 From 52f61e9f738de8e53b09e0237a23514a4249e370 Mon Sep 17 00:00:00 2001 From: Thomas Wiest Date: Wed, 3 Jun 2015 15:25:16 -0400 Subject: Added fail pattern rules to best practices doc --- docs/best_practices_guide.adoc | 46 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) (limited to 'docs/best_practices_guide.adoc') diff --git a/docs/best_practices_guide.adoc b/docs/best_practices_guide.adoc index 301c6ccda..14bd0bb53 100644 --- a/docs/best_practices_guide.adoc +++ b/docs/best_practices_guide.adoc @@ -19,7 +19,6 @@ This guide complies with https://www.ietf.org/rfc/rfc2119.txt[RFC2119]. | All pull requests MUST pass the build bot *before* they are merged. |=== - The purpose of this rule is to avoid cases where the build bot will fail pull requests for code modified in a previous pull request. The tooling is flexible enough that exceptions can be made so that the tool the build bot is running will ignore certain areas or certain checks, but the build bot itself must pass for the pull request to be merged. @@ -78,6 +77,49 @@ metadata[line] = results.pop() == Ansible +=== Defensive Programming + +.Context +* http://docs.ansible.com/fail_module.html[Ansible Fail Module] + +''' +[cols="2v,v"] +|=== +| **Rule** +| Ansible playbooks MUST begin with checks for any variables that they require. +|=== + +If an Ansible playbook requires certain variables to be set, it's best to check for these up front before any other actions have been performed. In this way, the user knows exactly what needs to be passed into the playbook. + +.Example: +[source,yaml] +---- +--- +- hosts: localhost + gather_facts: no + tasks: + - fail: msg="This playbook requires g_environment to be set and non empty" + when: g_environment is not defined or g_environment == '' +---- + +''' +[cols="2v,v"] +|=== +| **Rule** +| Ansible roles tasks/main.yml file MUST begin with checks for any variables that they require. +|=== + +If an Ansible role requires certain variables to be set, it's best to check for these up front before any other actions have been performed. In this way, the user knows exactly what needs to be passed into the role. + +.Example: +[source,yaml] +---- +--- +# tasks/main.yml +- fail: msg="This role requires arl_environment to be set and non empty" + when: arl_environment is not defined or arl_environment == '' +---- + === Roles .Context * http://docs.ansible.com/playbooks_best_practices.html#directory-layout[Ansible Suggested Directory Layout] @@ -101,7 +143,7 @@ metadata[line] = results.pop() | Ansible Roles SHOULD be named like technology_component[_subcomponent]. |=== -For clarity, it is suggested to follow a pattern when naming roles. It is important to note that this is a recommendation for role naming, and follows the pattern used by upstream. +For consistency, role names SHOULD follow the above naming pattern. It is important to note that this is a recommendation for role naming, and follows the pattern used by upstream. 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. -- cgit v1.2.3 From e43fc1b515fa87994228098e00632360ea5946b5 Mon Sep 17 00:00:00 2001 From: Thomas Wiest Date: Thu, 4 Jun 2015 10:08:06 -0400 Subject: Added YAML over JSON to best practices guide. --- docs/best_practices_guide.adoc | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) (limited to 'docs/best_practices_guide.adoc') diff --git a/docs/best_practices_guide.adoc b/docs/best_practices_guide.adoc index 2768059b3..841f6e72c 100644 --- a/docs/best_practices_guide.adoc +++ b/docs/best_practices_guide.adoc @@ -78,6 +78,27 @@ metadata[line] = results.pop() == Ansible +=== Yaml Files (Playbooks, Roles, Vars, etc) + +''' +[cols="2v,v"] +|=== +| **Rule** +| Ansible files SHOULD NOT use JSON (use pure YAML instead). +|=== + +YAML is a superset of JSON, which means that Ansible allows JSON syntax to be interspersed. Even though YAML (and by extension Ansible) allows for this, JSON SHOULD NOT be used. + +.Reasons: +* Ansible is able to give clearer error messages when the files are pure YAML +* YAML reads nicer (preference held by several team members) +* YAML makes for nicer diffs as YAML tends to be multi-line, whereas JSON tends to be more concise + +.Exceptions: +* Ansible static inventory files are INI files. To pass in variables for specific hosts, Ansible allows for these variables to be put inside of the static inventory files. These variables can be in JSON format, but can't be in YAML format. This is an acceptable use of JSON, as YAML is not allowed in this case. + +Every effort should be made to keep our Ansible YAML files in pure YAML. + === Defensive Programming .Context @@ -122,8 +143,6 @@ If an Ansible role requires certain variables to be set, it's best to check for ---- === Roles -.Context -* http://docs.ansible.com/playbooks_best_practices.html#directory-layout[Ansible Suggested Directory Layout] ''' [cols="2v,v"] @@ -132,6 +151,9 @@ If an Ansible role requires certain variables to be set, it's best to check for | The Ansible roles directory MUST maintain a flat structure. |=== +.Context +* http://docs.ansible.com/playbooks_best_practices.html#directory-layout[Ansible Suggested Directory Layout] + .The purpose of this rule is to: * Comply with the upstream best practices * Make it familiar for new contributors -- cgit v1.2.3