github.com/cilium/cilium@v1.16.2/Documentation/contributing/development/reviewers_committers/review_docs.rst (about) 1 .. only:: not (epub or latex or html) 2 3 WARNING: You are looking at unreleased Cilium documentation. 4 Please use the official rendered version released here: 5 https://docs.cilium.io 6 7 .. _review_docs: 8 9 ************************************ 10 Reviewing for @cilium/docs-structure 11 ************************************ 12 13 What is @cilium/docs-structure? 14 =============================== 15 16 Team `@cilium/docs-structure <docs-structure_team_>`_ is a GitHub team of 17 Cilium contributors who are responsible for maintaining the good state of the 18 project's documentation, by reviewing Pull Requests (PRs) that update the 19 documentation. Each time a non-draft PR touching files owned by the team opens, 20 GitHub automatically assigns one member of the team for review. 21 22 Open Cilium Pull Requests awaiting for reviews from @cilium/docs-structure are 23 `listed here <docs-structure_to_review_>`_. 24 25 To join the team, you must be a Cilium Reviewer. See `Cilium's Contributor 26 Ladder`_ for details on the requirements and the 27 application process. 28 29 .. _docs-structure_team: https://github.com/orgs/cilium/teams/docs-structure 30 .. _docs-structure_to_review: https://github.com/cilium/cilium/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse+team-review-requested%3Acilium%2Fdocs-structure 31 .. _Cilium's Contributor Ladder: https://github.com/cilium/community/blob/main/CONTRIBUTOR-LADDER.md 32 33 Reviewing Pull Requests 34 ======================= 35 36 This section describes some of the process and expectations for reviewing PRs 37 on behalf of cilium/docs-structure. Note that :ref:`the generic PR review 38 process for Committers <review_process>` applies, even though it is not 39 specific to documentation. 40 41 Technical contents 42 ------------------ 43 44 You are not expected to review the technical aspects of the documentation 45 changes in a PR. However, if you do have knowledge of the topic and if you find 46 some elements that are incorrect or missing, do flag them. 47 48 Documentation structure 49 ----------------------- 50 51 One essential part of a review is to ensure that the contribution maintains a 52 coherent structure for the documentation. Ask yourself if the changes are 53 located on the right page, at the right place. This is especially important if 54 pages are added, removed, or shuffled around. If the addition is large, 55 consider whether the page needs to split. Consider also whether new text comes 56 with a satisfactory structure. For example, does it fit well with the 57 surrounding context, or did the author simply use a "note" box instead of 58 trying to integrate the new information to the relevant paragraph? 59 60 See also :ref:`the recommendations on documentation structure for contributors 61 <docs_structure_recommendations>`. 62 63 Specific items to look out for 64 ------------------------------ 65 66 Backport labels 67 ~~~~~~~~~~~~~~~ 68 69 See :ref:`the backport criteria for documentation changes 70 <backport_criteria_docs>`. Mark the PR for backports by setting the labels for 71 all supported branches to which the changes apply, that is to say, all 72 supported branches containing the parent features to which the modified 73 sections relate. 74 75 CODEOWNERS updates 76 ~~~~~~~~~~~~~~~~~~ 77 78 All documentation sources are assigned to cilium/docs-structure for review by 79 default. However, when a contributor creates a new page, consider whether it 80 should be covered by another team as well so that this other team can review 81 the technical aspects. If this is the case, ask the author to update the 82 CODEOWNERS file. 83 84 Beta disclaimer 85 ~~~~~~~~~~~~~~~ 86 87 When a feature is advertised as Beta in the PR, make sure that the author 88 clearly indicates the Beta status in the documentation, both by mentioning 89 "(Beta)" in the heading of the section for the feature and by including the 90 dedicated banner, as follows: 91 92 .. code-block:: rst 93 94 .. include:: /Documentation/beta.rst 95 96 Upgrade notes 97 ~~~~~~~~~~~~~ 98 99 When the PR introduces new user-facing options, metrics, or behavior that 100 affects upgrades or downgrades, ensure that the author summarizes the changes 101 with a note in ``Documentation/operations/upgrade.rst``. 102 103 Completeness 104 ~~~~~~~~~~~~ 105 106 Make sure that new or updated content is complete, with no TODOs. 107 108 Auto-generated reference documents 109 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 110 111 When certain parts of the Cilium repository change, contributors may have to 112 update some auto-generated reference documents that are part of Cilium's 113 documentation, such as the command reference or the Helm reference. The CI 114 validates that these updates are present in the PR. If they are missing, you 115 may have to help contributors figure out what commands they need to run to 116 perform the updates. These commands are usually provided in the logs of the 117 GitHub workflows that failed to pass. 118 119 Spell checker exceptions 120 ~~~~~~~~~~~~~~~~~~~~~~~~ 121 122 The Documentation checks include running a spell checker. This spell checker 123 uses a file, ``Documentation/spelling_wordlist.txt``, containing a list of 124 spelling exceptions to ignore. Team cilium/docs-structure is the owner for this 125 file. Usually, there is not much feedback to provide on updates to the list of 126 exceptions. However, it's useful for reviewers to know that: 127 128 - Entries are sorted alphabetically, with all words starting with uppercase 129 letters coming before words starting with lowercase letters. 130 - Entries in the list of exceptions must be spelled correctly. 131 - Lowercase entries are case-insensitive for the spell checker, so reviewers 132 should reject new entries with capital letters if the lowercase versions 133 are already in the list. 134 135 Netlify preview 136 ~~~~~~~~~~~~~~~ 137 138 `Netlify`_ builds a new preview for each PR touching the documentation. You are 139 not expected to check the preview for each PR. However, if the PR contains 140 detailed formatting changes, such as nested blocks or directives, or changes to 141 tables or tabs, then it's good to validate that changes render as expected. 142 Also check the preview if you have a doubt as to the validity of the 143 reStructuredText (RST) mark-up that the author uses. 144 145 The list of checks on the PR page contains a link to the Netlify preview. If 146 the preview build failed, the link leads to the build logs. 147 148 .. _Netlify: https://www.netlify.com/?attr=homepage-modal 149 150 Formatting 151 ---------- 152 153 Read :ref:`Cilium's documentation style guide <docs_style_guide>`. 154 155 Flag poor formatting or obvious mistakes. The syntax for RST is not always 156 trivial and some contributors make mistakes, or they simply forget to use RST 157 and they employ Markdown mark-up instead. Make sure authors fix such issues. 158 159 Keep an eye on :ref:`code-blocks <docs_style_code_blocks>`: do they include RST 160 substitutions, and if so, do they use the right directive? If not, do they use 161 the right language? 162 163 Beyond that, the amount of time you spend on suggestions for improving 164 formatting is up to you. 165 166 Grammar and style 167 ----------------- 168 169 Read :ref:`Cilium's documentation style guide <docs_style_guide>`. 170 171 Flag obvious grammar mistakes. Try to read the updated text as a user would. 172 Ask the contributors to revise any sentence that is too difficult to read or to 173 understand. 174 175 @cilium/docs-structure aims to keep the documentation clean, consistent, and in 176 a clear and comprehensible state. User experience must always be as good as 177 possible. To achieve this objective, Documentation updates must follow best 178 practices, such as the ones from the style guide. Reviewing PRs at sufficient 179 depth to flag all potential style improvements can be time consuming, so the 180 amount of effort that you put into style guidance is up to you. 181 182 There is no tooling in place to enforce particular style recommendations. 183 184 Documentation build 185 =================== 186 187 The build framework 188 ------------------- 189 190 Here are the main resources involved or related to Cilium's documentation build 191 framework: 192 193 - :ref:`Instructions for building the documentation locally 194 <testing-documentation>` 195 - ``Documentation/Makefile``, ``Documentation/Dockerfile``, 196 ``Documentation/check-build.sh`` 197 - Dependencies are in ``Documentation/requirements.txt``, which is generated 198 from ``Documentation/requirements_min/requirements.txt`` 199 - The Sphinx theme we use is `our own fork <cilium_rtd_theme_>`_ of Read the 200 Docs's theme 201 202 .. _cilium_rtd_theme: https://github.com/cilium/sphinx_rtd_theme 203 204 Relevant CI workflows 205 --------------------- 206 207 Netlify preview 208 ~~~~~~~~~~~~~~~ 209 210 Documentation changes trigger the build of a new Netlify preview. If the build 211 fails, the PR authors or reviewers must investigate it. Ideally the author 212 should take care of this investigation, but in practice, contributors are not 213 always familiar with RST or with our build framework, so consider giving a 214 hand. 215 216 Documentation build 217 ~~~~~~~~~~~~~~~~~~~ 218 219 Same as the Netlify preview, the Documentation workflow runs on doc changes and 220 can raise missing updates on various generated pieces of documentation. 221 222 Checkpatch 223 ~~~~~~~~~~ 224 225 The Checkpatch workflow is part of the BPF tests and is not directly relevant 226 to documentation, but may raise some patch formatting issues, for example when 227 the commit title is too long. So it should run on doc-only PRs, like for any 228 other PR. 229 230 Integration tests 231 ~~~~~~~~~~~~~~~~~ 232 233 Integration tests, be it on Travis or on GitHub Actions, are the only workflows 234 that rebuild the ``docs-builder`` image. Building this image is necessary to 235 validate changes to the ``Documentation/Dockerfile`` or to the list of Python 236 dependencies located in ``Documentation/requirements.txt``. The GitHub workflow 237 uses a pre-built image instead, and won't incorporate changes to these files. 238 239 Integration tests also run a full build in the Cilium repository, including the 240 post-build checks, in particular ``Documentation/Makefile``'s ``check`` target. 241 Therefore, integration tests are able to raise inconsistencies in 242 auto-generated files in the documentation. 243 244 Ready to merge 245 -------------- 246 247 For PRs that only update documentation contents, the CI framework skips tests 248 that are not relevant to the changes. Therefore, authors or reviewers should 249 trigger the CI suite by commenting with ``/test``, just like for any other PR. 250 Once all code owners for the PR have approved, and all tests have passed, the 251 PR should automatically receive the ``ready-to-merge`` label.