Automating Ansible Code Reviews
02 June 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
- changes to standards cause version to be incremented
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
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
- As a commit hook (add
-q for just errors and warnings)
- ansible-review has helped in the review of around 10 internally developed ansible roles so far
- Few false positives.