Automating Ansible Code Reviews
25 October 2016
Why have code reviews?
- Spot bugs earlier
- Avoid unnecessary code (YAGNI)
- Encourage reuse (DRY)
- Avoid technical debt
Learning from reviews
- More heads => neater code
- Provides a means of building trust in new contributors
- Allows new contributors to learn from reviews
Why have standards?
- Consistency of code base, improves readability
- Rules are browsable, rather than found at review time.
- Standardise ahead of time, rather than arguing at review time
- Document best practices, and enforce them.
How do we manage standards?
- standards are testable, best practices are guidance
- review process for new standards
- standards are versioned (best practices are not)
Main rule of code reviews
- It is not permissable to block a code review on an undocumented rule.
- This means writing new rules as new best practices are 'discovered'
What standards exist?
- Ansible documents some Best practices for playbooks
- We have our own standards for consistency sake
- role versioning
- variable declaration location
Why automate reviews?
- Avoids the boring bits of code reviews
- Automated checks can be performed before even committing.
- Actual code reviews seem less pedantic
- Alexandra & Matt's talk on making the Right Way the Easy Way
- Have a versioned set of standards
- Test deployments against those standards
- Runs on individual files or even lines of individual files (good for diffs)
- Needs a set of standards rules to be defined
- Roles and playbooks must declare standards version (e.g.
# Standards: 1.2 in
- ansible-lint comes with a bunch of builtin rules (more with v3.0)
- Added some internal ansible-lint rules to our repo too.
- Based on ansible-lint checks
- Or write your own check in python
- A check takes a filename and settings
- And returns a
Result object, which is effectively a list of
How does it work?
- Each file in the input list is classified.
- The relevant rules for that classification are selected.
- The relevant rules are then applied to the file.
- Any resulting Errors are then filtered to match the lines being reviewed.
A sample standard rule
become_rather_than_sudo = Standard(dict(
name = "Use become/become_user/become_method rather than sudo/sudo_user",
check = lintcheck('ANSIBLE0008'),
types = ["playbook", "task"]
version = "0.9"
standards = [
For things that aren't yet standards but are worth knowing about (e.g. deprecated behaviour), a standard without a version will never error, only warn.
- Can review specific lines in set of files
ansible-review playbook.yml:14-18 otherthing.yml
- Can control which checks are important
- Can review older code against older versions for minor changes
git ls-files | xargs ansible-review
git diff master | ansible-review - to be implemented
- As a commit hook (add
-q for just errors)
Roadmap for ansible-lint (v3.5.0)
- Be the backend of choice for ansible-review rules for playbooks, tasks and handlers.
- Be useful for review rules for other file types.
- Add config file to allow new rules to be added without breaking existing setups (and then update the default config on minor version changes)
Roadmap for ansible-review (v0.13.0)
- Have a useful default set of standards
- Have a useful default set of lint rules that back those standards
- Move common standards code back into the main code base.
- ansible-review has helped in hundreds of reviews already.
- Few false positives (and those have often been bugs in the implementation rather than the idea).