guidelines coding - adeck/ansible-deployments GitHub Wiki
Coding Guidelines Index
Why Should I Read This?
Conforming to all the specifications here is a prerequisite for all code submissions. If you do not conform to all the (applicable) specifications listed here, your pull request will be summarily denied, you will be given the reason why, and you will be directed to this page. That having been said, lack of compliance to these guidelines is not the only grounds for denial of a pull request; these are simply the minimal requirements which are universally-applicable enough to merit explicit mention.
Guidelines are important for any coding project; they make it easy for new people to join, and make it easier for everyone to maintain and improve the existing codebase. Some rules are added for readability, some are for navigational ease, some are intended to increase modularity, others are intended for security. None of these guidelines are arbitrary; they all have specific, practical reasons behind them, even if (for conciseness' sake) those reasons aren't given alongside the rule.
This document covers coding guidelines, but there are also equally important documents which cover the guidelines for documentation, testing, and project structure. If you're doing any coding at all, reading the testing guidelines is mandatory, and if you're planning on writing / editing documentation, or writing any more code than a bugfix, reading the documentation guidelines is also mandatory.
Without further ado...
General Guidelines
- If it is impossible in a given circumstance to comply with a given requirement, document the exception and give a brief explanation of why it was necessary in both the commit comment, and (if you are writing a role) in a file called
exceptions.md
under thedoc
directory for that role. - Any pull request comment should begin with the string "REQUEST". Why? To make it easy to detect people who haven't read the guidelines.
- Commits must not be made to
master
until they have been tested, and the precise nature of that testing has been documented in the "testing" section of the wiki article for each role. See testing and documentation. At a bare minimum, the role must be run as part of a playbook twice against a clean install of the operating system it targets. The first run is executed to establish that the role performs its tasks successfully on initial run, the second is to establish the idempotence of the first run. - The correct / canonical way to begin a new project is to source
lib.sh
, cd intoprojects
, and runcreate <NAME_OF_PROJECT>
. This guarantees a baseline of compliance to project structure guidelines.
Code
All Code
- All lines must be 80 characters or fewer where possible.
- Raw tab characters,
\t
, should not be present anywhere. Most languages support the\t
escape sequence if it is necessary, including bash (via interpolated strings; specifically$'\t'
should work). - No line (including empty lines) should contain trailing whitespace.
- Indenting at each level should be two (2) spaces. Except in python, in which case you may use four spaces if PEP 8 compliance is really that important to you. But if you choose to go with PEP over this guide, I award you no points, and may god have mercy on your soul.
- You must put whitespace (specifically, one space) separating each operand from its adjacent operator. The
=
sign is an operator; it's the assignment operator. Similarly, a space must separate anif
orelif
from its conditional block, a space must also separate the first character of a comment from its "begin comment" operator, and (if a multiline comment) a space must separate the last character of the comment from its "end comment" operator. - Comments should always be on their own lines. They should precede the code they're documenting.
- Comments cannot replace issues in the issue tracker, or documentation in the wiki. The words
FIXME
andTODO
should not appear anywhere in code pushed tomaster
. This does not mean to simply delete them before committing; it means that you should create issue tickets instead, and in the comment where you would put aTODO
or aFIXME
, you provide a link to the related ticket. When you fix the problem, close the ticket, and remove the line. - Comments must be grammatically and factually correct.
Ansible-Specific
-
Task parameters should always be broken across lines; the
=
syntax should never be used. So, this is good:- name: install gcc yum: name: gcc state: present
While this is not:
- name: install gcc
yum: name=gcc state=present
If you need to perform a local_action
, it's worth noting that you can use the args
syntax to specify arguments as follows:
- local_action: file
args:
name: hello
state: directory
mode: 0500
- Packages should always be either
present
orabsent
; use oflatest
must be approved on a case-by-case basis, and should only be used in security hardening contexts. Where possible, package versioning should be left up to the person running the play, not the one writing the role. Naturally the line may become somewhat blurred in a role forspacewalk
,aptly
,pkgng
, etc. and that will be taken into account for such roles. - Any plaintext / config files managed by ansible should begin with an
{{ ansible_managed }}
comment if they are going to be copied to the remote. - Use
lower_snake_case
for variable names. Variables must be given names which reflect how they are used. This does not mean using role-specific prefixes, but it does mean that if a variable, which would otherwise have a general-seeming name, could only be used in a config file for a particular application, that application's name followed by an underscore should prefix the variable name. So if the applicationfoo
had atimeout
variable, you'd want to name the variablefoo_timeout
rather than simplytimeout
. - Always use an existing module where possible, giving preference to core modules over extensions, and giving preference to extensions over home-grown modules.
- There should be no ansible-generated warnings or errors from a production-ready playbook; syntactic, logical, or otherwise. So, all variables should be defined,
"{{ example_list }}"
syntax shouldn't be used with awith_items
. - Due, arguably, to a flaw in ansible,
include_vars
must not be used together withwith_items
because variables do not propagate correctly. - Unfortunately, within roles, a given
tasks
file will look for things like thefiles
directory relative to itself. In other words, if you have a filetasks/install/dependencies.yml
, and it includes acopy
task withsrc: hello.txt
, it will be looking intasks/files/hello.txt
and notfiles/hello.txt
within the role. This flaw is why we cannot have nice things. As such, while it is permissible to organize thetasks
directory into a hierarchical format, it is not okay to addfile
-related tasks (so, that includesscript
andcopy
, for example) anywhere but in the top level. The goal is to allow files / entire hierarchies to be shuffled around undertasks
without breaking everything by having hardcoded paths.