-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
docs: Add Committer Expectations document
Collect up all the committer expectations and PR requirements into a single place. Add additional guidelines about creating small PRs and how to break up PRs into multiple commits. Signed-off-by: Keith Short <keithshort@google.com>
- Loading branch information
1 parent
0a02a4a
commit 5d67ef7
Showing
4 changed files
with
261 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,248 @@ | ||
.. _committer-expectations: | ||
|
||
Committer Expectations | ||
###################### | ||
|
||
Overview | ||
******** | ||
|
||
The Zephyr project encourages you to submit changes as small pull requests. | ||
Small pull requests (PRs) have the following benefits: | ||
|
||
- Reviewed more quickly and reviewed more thoroughly. It's easier for reviewers | ||
to set aside a few minutes to review small changes several times than it is to | ||
allocate large block of time review a large PR. | ||
|
||
- Less wasted work if reviewers or maintainers reject the direction of the | ||
change. | ||
|
||
- Easier to rebase and merge. Smaller PRs are less likely to conflict with other | ||
changes in the tree. | ||
|
||
- Easier to revert if the PR breaks functionality. | ||
|
||
|
||
Defining Small PRs | ||
================== | ||
|
||
- Small PRs should encompass one self-contained logical change. | ||
|
||
- When adding a new large feature or API, the PR should address only one part of | ||
the feature. In this case create an :ref:`RFC proposal <rfcs>` [#rfc]_ to | ||
describe the additional parts of the feature for reviewers. | ||
|
||
- PRs should include tests for any added or changed lines of code. | ||
|
||
- PRs must update any documentation affected by the functional code changes. | ||
|
||
- If introducing a new API, the PR must include an example usage of the API. | ||
This provides context to the reviewer and prevents submitting PRs with unused | ||
APIs. | ||
|
||
|
||
Multiple Commits on a Single PR | ||
=============================== | ||
|
||
You are further encouraged to break up PRs into multiple commits. Keep in mind | ||
each commit of your PR should still build cleanly and pass all the CI tests. | ||
|
||
For example, when introducing an extension to an API, you can break up the PR | ||
into multiple commits targeting these specific changes: | ||
|
||
#. Introduce the new APIs, including shared devicetree bindings | ||
#. Update driver implementation X, with driver specific devicetree bindings | ||
#. Update driver implementation Y | ||
#. Add tests for the new API | ||
#. Add a sample using the API | ||
#. Update the documentation | ||
|
||
Large Changes | ||
============= | ||
|
||
Large changes to the Zephyr project must submit an :ref:`RFC proposal <rfcs>` | ||
describing the full scope of change and future work. The :ref:`RFC proposal | ||
<rfcs>` provides the required context to reviewers, but allows for smaller, | ||
incremental, PRs to get reviewed and merged into the project. The RFC should | ||
also define the minimum viable implementation. | ||
|
||
Changes which require an :ref:`RFC proposal <rfcs>` include: | ||
|
||
- Submitting a new feature. | ||
- Submitting a new API. | ||
- :ref:`treewide-changes`. | ||
- Other large changes that can benefit from the :ref:`RFC proposal <rfcs>` | ||
process. Maintainers have the discretion to request you create an RFC for your | ||
change. | ||
|
||
PR Requirements | ||
*************** | ||
|
||
- Each commit in the PR must provide a commit message following the | ||
:ref:`commit_message_guidelines`. | ||
|
||
- All files in the PR must comply with :ref:`Licensing | ||
Requirements<licensing_requirements>`. | ||
|
||
- Follow the Zephyr :ref:`coding_style` and :ref:`coding_guidelines`. | ||
|
||
- PRs must pass all CI checks. This is a requirement to merge the PR, but you | ||
may mark a PR as draft and explicitly request reviewers to provide early | ||
feedback. | ||
|
||
- When breaking a PR into multiple commits, each commit must build cleanly. The | ||
CI system does not enforce this policy, so it is the PR author's | ||
responsibility to verify. | ||
|
||
- When major new functionality is added, tests for the new functionality shall | ||
be added to the automated test suite. All API functions should have test cases | ||
and there should be tests for the behavior contracts of the API. Maintainers | ||
and reviewers have the discretion to determine if the provided tests are | ||
sufficient. The examples below demonstrate best practices on how to test APIs | ||
effectively. | ||
|
||
- `Kernel timer tests`_ provide better than 85% `test coverage`_ for the | ||
kernel timer. | ||
- TODO: Provide a driver level API test example. | ||
|
||
- Incompatible changes to APIs must also update the release notes for the | ||
next release detailing the change. APIs marked as experimental are excluded | ||
from this requirement. [#api-updates]_ | ||
|
||
- Changes to APIs must increment the API version number according to the API | ||
version rules. [#api-version]_ | ||
|
||
- PRs must also satisfy all :ref:`merge_criteria` before a member of the release | ||
engineering team merges the PR into the zephyr tree. | ||
|
||
Maintainers may request you break up a PR into smaller PRs and may request you | ||
create an :ref:`RFC proposal <rfcs>`. | ||
|
||
.. _Kernel timer tests: https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/kernel/timer/timer_behavior | ||
|
||
.. _test coverage: https://app.codecov.io/gh/zephyrproject-rtos/zephyr/blob/main/kernel/timer.c | ||
|
||
Workflow Suggestions That Help Reviewers | ||
======================================== | ||
|
||
- Don't resolve comments until the question has been answered or the request | ||
completed in an update to the PR. | ||
|
||
- Try to minimize rebases in the middle of a review. If a rebase is required, | ||
push this as a separate update with no functional changes to the PR. See the | ||
note below. | ||
|
||
- Request re-reviews once comments are addressed. [#re-review]_ | ||
|
||
- Instead of amending commits, append fixup commits to the PR to address | ||
reviewer comments. This provides reviewers a clear history of the changes made | ||
to the PR, along with the rationale for changes. Prior to the final approval | ||
and merge of the PR, you must perform a squash merge to cleanup the commit | ||
history. (TBD - This needs a PoC to demonstrate this workflow). | ||
|
||
PR Escalation | ||
============= | ||
|
||
The Zephyr community is a diverse group of individuals, with different levels of | ||
commitment and priorities. As such, reviewer and maintainers may not get to your | ||
PR right away. | ||
|
||
The Zephyr Project recommends the following approaches to escalate reviews for | ||
your PR. | ||
|
||
- After 2 days of inactivity, post a message on the `#pr-help`_ channel on | ||
Discord linking to your PR. | ||
|
||
- After 1 week of inactivity, add the `dev-review`_ label to your PR. This adds | ||
your PR to the agenda for the next `Zephyr Dev Meeting`_. | ||
|
||
- If reviewers on your PR propose mutually exclusive or otherwise incompatible | ||
changes, add your PR to the `Architecture Project`_. This marks your PR for | ||
triage by the `Architecture Working Group`_. [#arch-group]_ | ||
|
||
- In rare instances, the `Zephyr Dev Meeting`_ and `Architecture Working Group`_ | ||
may fail to reach consensus on the direction/design of a PR. In these cases, | ||
you may add the `tsc`_ label to your PR to escalate to the Zephyr Technical | ||
Steering Committee. | ||
|
||
- Members of the :ref:`release-engineering-team` have the authority to dismiss | ||
stale reviews if they determine the PR addressed the requested changes or | ||
the requested changes no longer apply. | ||
|
||
|
||
.. _#pr-help: https://discord.com/channels/720317445772017664/997527108844798012 | ||
|
||
.. _dev-review: https://github.com/zephyrproject-rtos/zephyr/labels/dev-review | ||
|
||
.. _Zephyr Dev Meeting: https://github.com/zephyrproject-rtos/zephyr/wiki/Zephyr-Committee-and-Working-Group-Meetings#zephyr-dev-meeting | ||
|
||
.. _Architecture Project: https://github.com/zephyrproject-rtos/zephyr/projects/18 | ||
|
||
.. _Architecture Working Group: https://github.com/zephyrproject-rtos/zephyr/wiki/Architecture-Working-Group | ||
|
||
.. _tsc: https://github.com/zephyrproject-rtos/zephyr/labels/tsc | ||
|
||
Reviewer Expectations | ||
##################### | ||
|
||
- Be respectful when commenting on PRs. Refer to the Zephyr `Code of Conduct`_ | ||
for more details. | ||
|
||
- The Zephyr Project recognizes that reviewers and maintainers have limited | ||
bandwidth. Prioritize review requests in the following order: | ||
|
||
#. PRs related to items in the `Zephyr Release Plan`_. | ||
#. PRs you previously reviewed and requested blocking changes. | ||
#. PRs assigned to you as the area maintainer. | ||
#. All other PRs. | ||
|
||
- Try to provide feedback on the entire PR in one shot. This provides the author | ||
an opportunity to address all comments in the next PR update. | ||
|
||
- Partial reviews are permitted, but the reviewer must add a comment indicating | ||
what portion of the PR they reviewed. Examples of useful partial reviews | ||
include: | ||
|
||
- Domain specific reviews (e.g. Devicetree). | ||
- Code style changes that impact the readability of the PR. | ||
- Reviewing commits separately when the requested changes cascade into the | ||
later commits. | ||
|
||
- Avoid increasing scope of the PR by requesting new features, especially when | ||
there is a corresponding :ref:`RFC <rfcs>` associated with the PR. Instead add | ||
your suggestions as a comment to the :ref:`RFC <rfcs>`. This also encourages | ||
more collaboration as it is easier for multiple contributors to work on a | ||
feature once the minimum implementation has merged. | ||
|
||
- Mark trivial, non-functional, requests as "Non-blocking" in your comment. | ||
Reviewers should approve PRs once only non-blocking changes remain. The PR | ||
author has discretion as to whether they address all non-blocking comments. | ||
|
||
.. _Code of Conduct: https://github.com/zephyrproject-rtos/zephyr/blob/main/CODE_OF_CONDUCT.md | ||
|
||
.. _Zephyr Release Plan: https://github.com/orgs/zephyrproject-rtos/projects/13 | ||
|
||
.. rubric:: Footnotes | ||
|
||
.. [#rfc] | ||
The RFC Proposals doc currently lives under Project Governance. Is there a | ||
better location for this doc? | ||
.. [#api-updates] | ||
This idea was proposed in the Process Improvement working group, but has | ||
not yet been approved. | ||
.. [#api-version] | ||
API versioning is still pending a TSC vote. | ||
.. [#re-review] | ||
Are there access controls that block some users from this action? | ||
.. [#arch-group] | ||
This is the process is documented on the Architecture Working Group wiki | ||
page, but in practice the agenda seems to be only set by email Carles | ||
directly. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters