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

---
# SPDX-License-Identifier: Apache-2.0
# SPDX-FileCopyrightText: 2018 The Linux Foundation

default_language_version:
  python: python3

repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: cef0300fd0fc4d2a87a85fa2093c6b283ea36f4b  # frozen: v5.0.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: acc9d9de6369b76d22cb4167029d2035e8730b98  # frozen: v0.19.1
    hooks:
      - id: gitlint

  - repo: https://github.com/astral-sh/ruff-pre-commit
    rev: 41d2daf392fbf43341a77f24a9c6481a4b66af98  # frozen: v0.9.10
    hooks:
      - id: ruff
        files: ^(docs)/.+\.py$
        args: [--fix, --exit-non-zero-on-fix]
      - id: ruff-format
        files: ^(docs)/.+\.py$


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

  - repo: https://github.com/pycqa/pydocstyle
    rev: 07f6707e2c5612960347f7c00125620457f490a7  # frozen: 6.3.0
    hooks:
      - id: pydocstyle


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

  - repo: https://github.com/rhysd/actionlint
    rev: 03d0035246f3e81f36aed592ffb4bebf33a03106  # frozen: v1.7.7
    hooks:
      - id: actionlint

  - repo: https://github.com/fsfe/reuse-tool
    rev: 60dfc6b2ad9e1f3eabfbcf3a0dc202ee89dc5a00  # frozen: v5.0.2
    hooks:
      - id: reuse

tox.ini

# SPDX-License-Identifier: Apache-2.0
# SPDX-FileCopyrightText: 2017 The Linux Foundation

[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

1. 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

2. Clone your repo

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

3. Setup an upstream remote

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

4. Create local branch and do your work (same as with gerrit)

   git checkout -b $feature
   

5. Push your local branch to your fork, preferably as a branch on your fork

   git push origin $feature
   

6. 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.

1. Fetch upstream changes from the remote you’ve already added:

   git fetch upstream
   

2. Switch to primary branch and refresh your master branch

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

3. Update Github with your synced fork:

   git push origin