Reviewing for @cilium/docs-structure
What is @cilium/docs-structure?
Team @cilium/docs-structure is a GitHub team of Cilium contributors who are responsible for maintaining the good state of the project’s documentation, by reviewing Pull Requests (PRs) that update the documentation. Each time a non-draft PR touching files owned by the team opens, GitHub automatically assigns one member of the team for review.
Open Cilium Pull Requests awaiting for reviews from @cilium/docs-structure are listed here.
To join the team, you must be a Cilium Reviewer. See Cilium’s Contributor Ladder for details on the requirements and the application process.
Reviewing Pull Requests
This section describes some of the process and expectations for reviewing PRs on behalf of cilium/docs-structure. Note that the generic PR review process for Committers applies, even though it is not specific to documentation.
Technical contents
You are not expected to review the technical aspects of the documentation changes in a PR. However, if you do have knowledge of the topic and if you find some elements that are incorrect or missing, do flag them.
Documentation structure
One essential part of a review is to ensure that the contribution maintains a coherent structure for the documentation. Ask yourself if the changes are located on the right page, at the right place. This is especially important if pages are added, removed, or shuffled around. If the addition is large, consider whether the page needs to split. Consider also whether new text comes with a satisfactory structure. For example, does it fit well with the surrounding context, or did the author simply use a “note” box instead of trying to integrate the new information to the relevant paragraph?
See also the recommendations on documentation structure for contributors.
Specific items to look out for
Backport labels
See the backport criteria for documentation changes. Mark the PR for backports by setting the labels for all supported branches to which the changes apply, that is to say, all supported branches containing the parent features to which the modified sections relate.
CODEOWNERS updates
All documentation sources are assigned to cilium/docs-structure for review by default. However, when a contributor creates a new page, consider whether it should be covered by another team as well so that this other team can review the technical aspects. If this is the case, ask the author to update the CODEOWNERS file.
Beta disclaimer
When a feature is advertised as Beta in the PR, make sure that the author clearly indicates the Beta status in the documentation, both by mentioning “(Beta)” in the heading of the section for the feature and by including the dedicated banner, as follows:
.. include:: /Documentation/beta.rst
Upgrade notes
When the PR introduces new user-facing options, metrics, or behavior that
affects upgrades or downgrades, ensure that the author summarizes the changes
with a note in Documentation/operations/upgrade.rst.
Completeness
Make sure that new or updated content is complete, with no TODOs.
Auto-generated reference documents
When certain parts of the Cilium repository change, contributors may have to update some auto-generated reference documents that are part of Cilium’s documentation, such as the command reference or the Helm reference. The CI validates that these updates are present in the PR. If they are missing, you may have to help contributors figure out what commands they need to run to perform the updates. These commands are usually provided in the logs of the GitHub workflows that failed to pass.
Spell checker exceptions
The Documentation checks include running a spell checker. This spell checker
uses a file, Documentation/spelling_wordlist.txt, containing a list of
spelling exceptions to ignore. Team cilium/docs-structure is the owner for this
file. Usually, there is not much feedback to provide on updates to the list of
exceptions. However, it’s useful for reviewers to know that:
Entries are sorted alphabetically, with all words starting with uppercase letters coming before words starting with lowercase letters.
Entries in the list of exceptions must be spelled correctly.
Lowercase entries are case-insensitive for the spell checker, so reviewers should reject new entries with capital letters if the lowercase versions are already in the list.
Netlify preview
Netlify builds a new preview for each PR touching the documentation. You are not expected to check the preview for each PR. However, if the PR contains detailed formatting changes, such as nested blocks or directives, or changes to tables or tabs, then it’s good to validate that changes render as expected. Also check the preview if you have a doubt as to the validity of the reStructuredText (RST) mark-up that the author uses.
The list of checks on the PR page contains a link to the Netlify preview. If the preview build failed, the link leads to the build logs.
Formatting
Read Cilium’s documentation style guide.
Flag poor formatting or obvious mistakes. The syntax for RST is not always trivial and some contributors make mistakes, or they simply forget to use RST and they employ Markdown mark-up instead. Make sure authors fix such issues.
Keep an eye on code-blocks: do they include RST substitutions, and if so, do they use the right directive? If not, do they use the right language?
Beyond that, the amount of time you spend on suggestions for improving formatting is up to you.
Grammar and style
Read Cilium’s documentation style guide.
Flag obvious grammar mistakes. Try to read the updated text as a user would. Ask the contributors to revise any sentence that is too difficult to read or to understand.
@cilium/docs-structure aims to keep the documentation clean, consistent, and in a clear and comprehensible state. User experience must always be as good as possible. To achieve this objective, Documentation updates must follow best practices, such as the ones from the style guide. Reviewing PRs at sufficient depth to flag all potential style improvements can be time consuming, so the amount of effort that you put into style guidance is up to you.
There is no tooling in place to enforce particular style recommendations.
Documentation build
The build framework
Here are the main resources involved or related to Cilium’s documentation build framework:
Documentation/Makefile,Documentation/Dockerfile,Documentation/check-build.shDependencies are in
Documentation/requirements.txt, which is generated fromDocumentation/requirements_min/requirements.txtThe Sphinx theme we use is our own fork of Read the Docs’s theme
Relevant CI workflows
Netlify preview
Documentation changes trigger the build of a new Netlify preview. If the build fails, the PR authors or reviewers must investigate it. Ideally the author should take care of this investigation, but in practice, contributors are not always familiar with RST or with our build framework, so consider giving a hand.
Documentation build
Same as the Netlify preview, the Documentation workflow runs on doc changes and can raise missing updates on various generated pieces of documentation.
Checkpatch
The Checkpatch workflow is part of the BPF tests and is not directly relevant to documentation, but may raise some patch formatting issues, for example when the commit title is too long. So it should run on doc-only PRs, like for any other PR.
Integration tests
Integration tests, be it on Travis or on GitHub Actions, are the only workflows
that rebuild the docs-builder image. Building this image is necessary to
validate changes to the Documentation/Dockerfile or to the list of Python
dependencies located in Documentation/requirements.txt. The GitHub workflow
uses a pre-built image instead, and won’t incorporate changes to these files.
Integration tests also run a full build in the Cilium repository, including the
post-build checks, in particular Documentation/Makefile’s check target.
Therefore, integration tests are able to raise inconsistencies in
auto-generated files in the documentation.
Ready to merge
For PRs that only update documentation contents, the CI framework skips tests
that are not relevant to the changes. Therefore, authors or reviewers should
trigger the CI suite by commenting with /test, just like for any other PR.
Once all code owners for the PR have approved, and all tests have passed, the
PR should automatically receive the ready-to-merge label.