github.com/cilium/cilium@v1.16.2/Documentation/contributing/development/reviewers_committers/review_process.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_process: 8 9 Pull requests review process for committers 10 =========================================== 11 12 Review process 13 -------------- 14 15 .. note:: 16 17 These instructions assume that reviewers are members of the Cilium GitHub 18 organization. This is required to obtain the privileges to modify GitHub 19 labels on the pull request. See `Cilium's Contributor Ladder`_ for details. 20 21 .. _Cilium's Contributor Ladder: https://github.com/cilium/community/blob/main/CONTRIBUTOR-LADDER.md 22 23 #. Find Pull Requests (PRs) needing a review `from you <user_review_filter_>`_, 24 or `from one of your teams <team_review_filter_>`_. 25 26 #. If this PR was opened by a contributor who is not part of the Cilium 27 organization, please assign yourself to that PR and keep track of the PR to 28 ensure it gets reviewed and merged. 29 30 If the contributor is a Cilium committer, then they are responsible for 31 getting the PR in a ready to be merged state by adding the 32 ``ready-to-merge`` label, once all reviews have been addressed and CI checks 33 are successful, so that they (or another committer) can merge the PR. 34 35 If this PR is a backport PR (typically with the label ``kind/backport``) and 36 no-one else has reviewed the PR, review the changes as a sanity check. If 37 any individual commits deviate from the original patch, request review from 38 the original author to validate that the backport was correctly applied. 39 40 #. Review overall correctness of the PR according to the rules specified in the 41 section :ref:`submit_pr`. 42 43 #. Set the labels accordingly. A bot called maintainer's little helper might 44 automatically help you with this. 45 46 +--------------------------------+---------------------------------------------------------------------------+ 47 | Labels | When to set | 48 +================================+===========================================================================+ 49 | ``dont-merge/needs-sign-off`` | Some commits are not signed off | 50 +--------------------------------+---------------------------------------------------------------------------+ 51 | ``needs-rebase`` | PR is outdated and needs to be rebased | 52 +--------------------------------+---------------------------------------------------------------------------+ 53 54 #. Validate that bugfixes are marked with ``kind/bug`` and validate whether the 55 assessment of backport requirements as requested by the submitter conforms 56 to the :ref:`backport_criteria`. 57 58 +--------------------------+---------------------------------------------------------------------------+ 59 | Labels | When to set | 60 +==========================+===========================================================================+ 61 | ``needs-backport/X.Y`` | PR needs to be backported to these stable releases | 62 +--------------------------+---------------------------------------------------------------------------+ 63 64 #. If the PR is subject to backport, validate that the PR does not mix bugfix 65 and refactoring of code as it will heavily complicate the backport process. 66 Demand for the PR to be split. 67 68 #. Validate the ``release-note/*`` label and check the release note 69 suitability. Release notes are passed through the dedicated ``release-note`` 70 block (see :ref:`submit_pr`), or through the PR title if this block is 71 missing. To check if the notes are suitable, put yourself into the 72 perspective of a future release notes reader with lack of context and ensure 73 the title is precise but brief. 74 75 +-----------------------------------+--------------------------------------------------------------------------------------------------------+ 76 | Labels | When to set | 77 +===================================+========================================================================================================+ 78 | ``dont-merge/needs-release-note`` | Do NOT merge PR, needs a release note | 79 +-----------------------------------+--------------------------------------------------------------------------------------------------------+ 80 | ``release-note/bug`` | This is a non-trivial bugfix and is a user-facing bug | 81 +-----------------------------------+--------------------------------------------------------------------------------------------------------+ 82 | ``release-note/major`` | This is a major feature addition, e.g. Add MongoDB support | 83 +-----------------------------------+--------------------------------------------------------------------------------------------------------+ 84 | ``release-note/minor`` | This is a minor feature addition, e.g. Add support for a Kubernetes version | 85 +-----------------------------------+--------------------------------------------------------------------------------------------------------+ 86 | ``release-note/misc`` | This is a not user-facing change , e.g. Refactor endpoint package, a bug fix of a non-released feature | 87 +-----------------------------------+--------------------------------------------------------------------------------------------------------+ 88 | ``release-note/ci`` | This is a CI feature or bug fix. | 89 +-----------------------------------+--------------------------------------------------------------------------------------------------------+ 90 91 #. Check for upgrade compatibility impact and if in doubt, set the label 92 ``upgrade-impact`` and discuss in `Cilium Slack`_'s ``#development`` channel 93 or in the weekly meeting. 94 95 +--------------------------+---------------------------------------------------------------------------+ 96 | Labels | When to set | 97 +==========================+===========================================================================+ 98 | ``upgrade-impact`` | The code changes have a potential upgrade impact | 99 +--------------------------+---------------------------------------------------------------------------+ 100 101 #. When submitting a review, provide explicit approval or request specific 102 changes whenever possible. Clear feedback indicates whether contributors 103 must take action before a PR can merge. 104 105 If you need more information before you can approve or request changes, you 106 can leave comments seeking clarity. If you do not explicitly approve or 107 request changes, it's best practice to raise awareness about the discussion 108 so that others can participate. Here are some ways you can raise awareness: 109 110 - Re-request review from codeowners in the PR 111 - Raise the topic for discussion in Slack or during community meetings 112 113 When requesting changes, summarize your feedback for the PR, including 114 overall issues for a contributor to consider and/or encouragement for what a 115 contributor is already doing well. 116 117 #. When all review objectives for all ``CODEOWNERS`` are met, all CI tests have 118 passed, and all reviewers have approved the requested changes, you can merge 119 the PR by clicking on the "Rebase and merge" button. 120 121 Reviewer Teams 122 -------------- 123 124 Every reviewer, including committers in the `committers team`_, belongs to `one 125 or more teams in the Cilium organization <cilium_teams_>`_. If you would like 126 to add or remove yourself from any team, please submit a PR against the 127 `community repository`_. 128 129 Once a contributor opens a PR, GitHub automatically picks which `teams 130 <cilium_teams_>`_ should review the PR using the ``CODEOWNERS`` file. Each 131 reviewer can see the PRs they need to review by filtering by reviews 132 requested. A good filter is provided in this `link <user_review_filter_>`_ so 133 make sure to bookmark it. 134 135 Reviewers are expected to focus their review on the areas of the code where 136 GitHub requested their review. For small PRs, it may make sense to simply 137 review the entire PR. However, if the PR is quite large then it can help to 138 narrow the area of focus to one particular aspect of the code. When leaving a 139 review, share which areas you focused on and which areas you think that other 140 reviewers should look into. This helps others to focus on aspects of review 141 that have not been covered as deeply. 142 143 Belonging to a team does not mean that a reviewer needs to know every single 144 line of code the team is maintaining. Once you have reviewed a PR, if you feel 145 that another pair of eyes is needed, re-request a review from the appropriate 146 team. In the following example, the reviewer belonging to the CI team is 147 re-requesting a review for other team members to review the PR. This allows 148 other team members belonging to the CI team to see the PR as part of the PRs 149 that require review in the `filter <team_review_filter_>`_. 150 151 .. image:: ../../../images/re-request-review.png 152 :align: center 153 :scale: 50% 154 155 When all review objectives for all ``CODEOWNERS`` are met, all required CI 156 tests have passed and a proper release label is set, you may set the 157 ``ready-to-merge`` label to indicate that all criteria have been met. 158 Maintainer's little helper might set this label automatically if the previous 159 requirements were met. 160 161 +--------------------------+---------------------------+ 162 | Labels | When to set | 163 +==========================+===========================+ 164 | ``ready-to-merge`` | PR is ready to be merged | 165 +--------------------------+---------------------------+ 166 167 .. _committers team: https://github.com/orgs/cilium/teams/committers/members 168 .. _community repository: https://github.com/cilium/community 169 .. _cilium_teams: https://github.com/orgs/cilium/teams/team/teams 170 .. _maintainers: https://github.com/orgs/cilium/teams/cilium-maintainers/members 171 .. _user_review_filter: https://github.com/cilium/cilium/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse+user-review-requested%3A%40me+sort%3Aupdated-asc 172 .. _team_review_filter: https://github.com/cilium/cilium/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse+review-requested%3A%40me+sort%3Aupdated-asc 173 174 Code owners 175 ----------- 176 177 .. include:: ../../../codeowners.rst