Best Practices¶

Code Review¶

All patches that go into a project repo need to be code reviewed by someone other than the original author. Code review is a great way to both learn from others as well as improve code quality. Contribution to code review is highly recommended regardless of activity as a committer.

Below provides a simple checklist of common items that code reviewers should look out for (Patch submitters can use this to self-review as well to ensure that they are not hitting any of these):

General

Does the Git commit message sufficiently describe the change? (Refer to: https://chris.beams.io/posts/git-commit/ and https://fatbusinessman.com/2019/my-favourite-git-commit)
Does the Git commit subject line need to follow the Conventional Commit specification? (Refer to: https://www.conventionalcommits.org/ and https://gist.github.com/joshbuchea/6f47e86d2510bce28f8e7f42ae84c716)
If the change has an associated issue tracker, does the commit message have an ‘Issue: <someissue>’ (or any similar tag such as ‘Issue-Id:’ or ‘JIRA:’) in the footer and not in the subject line or body?
Is all meta-data in the footer? This includes the above point and any other key-value data pairings that are truly meta-data. Such as, but not limited to, Signed-off-by, Change-Id, Issue, Jira, Issue-Id, Bug, etc.
Are there any typos?
Are all code review comments addressed?
Is the code rebased onto the latest HEAD of the branch?
Does the code pull in any dependencies that might have license conflicts with this project’s license?
Is the Git commit body independent from the title ? The first paragraph should not be a continued flow from the subject line but a paragraph that can stand on its own.
If important changes are brought by the commit(s), does an appropriate ChangeLog file need to be created ? (for example a reno YAML file for Releng)

Code

Are imports alphabetical and sectioned off by stdlib, 3rdparty, and local?
Are functions / methods organized alphabetically? (or categorized alphabetically)
Does the change need unit tests? (Yes, it probably does!)
Does the change need documentation?
Does every function added have function docs? (javadoc, pydoc, whatever the language code docs is)
Does it pass linting?
Does the code cause backwards compatibility breakage? (If so it needs documentation)

Note

Refer to Google’s blog (google-blog-code-health) on effective code review.

Generic Linting (pre-commit)¶

pre-commit is a Git hooks management tool and is great for running linters from any code languages. The easiest way to run pre-commit is with python-tox and requires Python 3 installed on the system:

tox -epre-commit

However if you want a more automated experience we recommend running pre-commit directly and installing the hooks such that they automatically run when you execute the git commit command. In this case, install pre-commit using your package manager or pip install it if your distro does not have it available.

Requirements¶

Python 3
Python pre-commit

Install pre-commit¶

If pre-commit is not available from your native package manager than you can install it via Python’s pip install command:

pip install --user pre-commit
pre-commit --help

Once installed for every repo that are are working on you can install the pre-commit hooks directly into your local Git hooks on a per repo basis.

pre-commit install

Set up pre-commit for a Project¶

Requirements

Python 3
Python pre-commit
Python Tox

Configure the project with a tox.ini and a .pre-commit-config.yaml file. Below are examples of .pre-commit-config.yaml and tox.ini as defined by this project. Inside the tox.ini file the interesting bits are under [testenv:pre-commit].

.pre-commit-config.yaml

---
default_language_version:
  python: python3

repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.4.0
    hooks:
      - id: check-executables-have-shebangs
      - id: check-merge-conflict
      - id: end-of-file-fixer
      - id: trailing-whitespace

  - repo: https://github.com/jorisroovers/gitlint
    rev: v0.18.0
    hooks:
      - id: gitlint

  - repo: https://github.com/ambv/black
    rev: 23.3.0
    hooks:
      - id: black

  - repo: https://github.com/PyCQA/flake8
    rev: 6.0.0
    hooks:
      - id: flake8
        args: ["--max-line-length=120"]
        additional_dependencies: [Flake8-pyproject]

  - repo: https://github.com/pycqa/bandit
    rev: 1.7.5
    hooks:
      - id: bandit
        # Bandit does not need to run on test code
        exclude: tests/*

  - repo: https://github.com/pycqa/pydocstyle
    rev: 6.3.0
    hooks:
      - id: pydocstyle


  - repo: https://github.com/btford/write-good
    rev: v1.0.8
    hooks:
      - id: write-good
        files: "\\.(rst|md|markdown|mdown|mkdn)$"
        exclude: docs/infra/gerrit.rst|docs/best-practices.rst

tox.ini

[tox]
minversion = 1.6
envlist =
    check-best-practices,
    docs,
    docs-linkcheck,
    pre-commit
skipsdist=true

[testenv]
basepython = python3
install_command=python -m pip install --no-cache-dir {opts} {packages}

[testenv:check-best-practices]
basepython = python3
commands = python {toxinidir}/check-best-practices.py

[testenv:docs]
basepython = python3
deps = -rrequirements.txt
commands =
    sphinx-build -q -j auto -W -b html -n -W -d {envtmpdir}/doctrees ./docs/ {toxinidir}/docs/_build/html

[testenv:docs-linkcheck]
basepython = python3
deps = -rrequirements.txt
commands = sphinx-build -q -j auto -W -b linkcheck -d {envtmpdir}/doctrees ./docs/ {toxinidir}/docs/_build/linkcheck

[testenv:pre-commit]
basepython = python3
allowlist_externals =
    /bin/sh
deps =
    pre-commit
passenv = HOME
commands =
    pre-commit run --all-files --show-diff-on-failure
    /bin/sh -c 'if ! git config --get user.name > /dev/null; then \
        git config --global --add user.name "CI"; \
        touch .git/REMOVE_USERNAME; fi'
    /bin/sh -c 'if ! git config --get user.email > /dev/null; then \
        git config --global --add user.email "ci@example.org"; \
        touch .git/REMOVE_USEREMAIL; fi'
    /bin/sh -c "if [ -f .git/COMMIT_EDITMSG ]; then \
        cp .git/COMMIT_EDITMSG .git/COMMIT_MSGTOX; else \
        git log HEAD -n1 | tail -n +5 | cut -c 5- > .git/COMMIT_MSGTOX; fi"
    pre-commit run gitlint --hook-stage commit-msg --commit-msg-filename .git/COMMIT_MSGTOX
    /bin/sh -c "rm -f .git/COMMIT_MSGTOX"
    /bin/sh -c "if [ -f .git/REMOVE_USERNAME ]; then \
        git config --global --unset user.name; \
        rm -f .git/REMOVE_USERNAME; fi"
    /bin/sh -c "if [ -f .git/REMOVE_USEREMAIL ]; then \
        git config --global --unset user.email; \
        rm -f .git/REMOVE_USEREMAIL; fi"

Jenkins Job Builder¶

Jenkins Job Builder Best Practices

GitHub Workflow¶

When working directly on Github (as opposed to Gerrit systems mirrored to Github), you’ll need to create a fork and use branches/ pull requests to get changes merged to the main repo. Here are some instructions on creating and maintaining your fork.

Forking and working¶

Fork your $PROJECT/$REPO to your personal Github account
- NOTE if you are forking the ci-management repository you should consider
  changing the local fork name after you have forked it to be $PROJECT-ci-management this has 2 benefits:
  1. Let you know which upstream project the ci-management repo it’s for
  2. Allow you to fork the next ci-management repository that you might need to work on

Clone your repo

git clone git@github.com:$MYACCOUNT/$REPO

Setup an upstream remote

git remote add upstream git@github.com:$PROJECT/$REPO

Create local branch and do your work (same as with gerrit)
```
git checkout -b $feature
```
Push your local branch to your fork, preferably as a branch on your fork
```
git push origin $feature
```
Raise PR against the upstream (note: when pushing a branch from your local to your fork the CLI gives you a URL for raising the PR)

Care and feeding of your fork¶

Your fork will fall out of sync with the upstream repo so be sure to tend: to your fork before working on it.

Fetch upstream changes from the remote you’ve already added:
```
git fetch upstream
```

Switch to primary branch and refresh your master branch

git checkout master && git pull --rebase upstream master

Update Github with your synced fork:
```
git push origin
```