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>

doc/contribute/committer_expectations.rst

@@ -0,0 +1,287 @@
+.. _committer-expectations:
+
+Committer Expectations
+######################
+
+Overview
+********
+
+The Zephyr project encourages committers to submit changes as smaller pull
+requests. Smaller 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 smaller 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 Smaller PRs
+====================
+
+- Smaller 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>` to describe the
+  additional parts of the feature for reviewers.
+
+- PRs should include tests or samples under the following conditions:
+
+  - Adding new features or functionality.
+
+  - Modifying a feature, especially for API behavior contract changes.
+
+  - Fixing a hardware agnostic bug. The test should fail without the bug fixed
+    and pass with the fix applied.
+
+- 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
+===============================
+
+Committers are further encouraged to break up PRs into multiple commits.  Keep
+in mind each commit in the PR must still build cleanly and pass all the CI
+tests.
+
+For example, when introducing an extension to an API, Committers 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 RFC proposal 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 RFC proposal include:
+
+- Submitting a new feature.
+- Submitting a new API.
+- :ref:`treewide-changes`.
+- Other large changes that can benefit from the RFC proposal process.
+
+Maintainers have the discretion to request committers create an RFC for PRs that
+are too large or complicated.
+
+PR Requirements
+***************
+
+- Each commit in the PR must provide a commit message following the
+  :ref:`commit-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. Committers
+  may mark a PR as draft and explicitly request reviewers to provide early
+  feedback, even with failing CI checks.
+
+- 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.
+    - Emulators for off-chip peripherals are an effective way to test driver
+      APIs. The `smart battery sensor tests`_ use the `smart battery emulator`_,
+      providing test coverage for the `fuel gauge API`_ and the `smart battery
+      sensor driver`_
+
+
+- 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 committers break up a PR into smaller PRs and may
+request the committer 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
+
+.. _`smart battery sensor tests`: https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/sensor/sbs_gauge/src/test_sbs_gauge.c
+
+.. _`smart battery emulator`: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/sensor/sbs_gauge/emul_sbs_gauge.c
+
+.. _`fuel gauge API`: https://app.codecov.io/gh/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/fuel_gauge.h
+
+.. _`smart battery sensor driver`: https://app.codecov.io/gh/zephyrproject-rtos/zephyr/blob/main/drivers/sensor/sbs_gauge/sbs_gauge.c
+
+Workflow Suggestions That Help Reviewers
+========================================
+
+- Unless they applied the reviewer's recommendation exactly, authors must not
+  resolve and hide comments, they must let the initial reviewer do it. The
+  Zephyr project does not require all comments to be resolved before merge.
+  Leaving some completed discussions open can sometimes be useful to understand
+  the greater picture.
+
+- Respond to comments using the "Start Review" and "Add Review" green buttons in
+  the "Files changed" view. This allows you to respond to multiple comments and
+  publish the responses in bulk. This reduces the number of emails sent to
+  reviewers.
+
+- Try to minimize rebases in the middle of a review. If a rebase is required,
+  push this as a separate update with no other changes since the last push of
+  the PR. When pushing a rebase only, add a comment to the PR indicating which
+  commit is the rebase.
+
+PR Review Escalation
+====================
+
+The Zephyr community is a diverse group of individuals, with different levels of
+commitment and priorities. As such, reviewers and maintainers may not get to a
+PR right away.
+
+The `Zephyr Dev Meeting`_ performs a triage of PRs missing reviewer approval,
+following this process:
+
+#. Identify and update PRs missing an Assignee.
+#. Identify PRs without any comments or reviews, ping the PR Assignee to start a
+   review or assign to a different maintainer.
+#. For PRs that have otherwise stalled, the Zephyr Dev Meeting pings the
+   Assignee and any reviewers that have left comments on the PR.
+
+Committers may escalate PRs outside of the Zephyr Dev Meeting triage process as
+follows:
+
+- After 1 week of inactivity, ping the Assignee or reviewers on the PR by adding
+  a comment to the PR.
+
+- After 2 weeks of inactivity, post a message on the `#pr-help`_ channel on
+  Discord linking to the PR.
+
+- After 2 weeks of inactivity, add the `dev-review`_ label to the PR. This
+  explicitly adds the PR to the agenda for the next `Zephyr Dev Meeting`_
+  independent of the triage process. Not all contributors have the required
+  privileges to add labels to PRs, in this case the committer should request
+  help on Discord or send an email to the `Zephyr devel mailing list`_.
+
+Note that for new PRs, committers should generally wait for at least one Zephyr
+Dev Meeting before escalating the PR themselves.
+
+.. _Zephyr devel mailing list: https://lists.zephyrproject.org/g/devel
+
+
+.. _pr_technical_escalation:
+
+PR Technical Escalation
+=======================
+
+In cases where a committer objects to change requests from reviewers, Zephyr
+defines the following escalation process for resolving technical disagreements.
+
+- Resolve in the PR among assignee, maintainers and reviewer.
+
+  - Assignee to act as moderator if applicable.
+
+- Optionally resolve in the next `Zephyr Dev Meeting`_ or `Architecture Working
+  Group`_ meeting with more Maintainers and project stakeholders.
+
+  - The involved parties and the Assignee to be present when
+    the (escalated) issue is discussed.
+
+- TSC: Assignees can escalate to the TSC voting members and get a binding
+  resolution in the TSC by adding the `tsc`_ label on the PR.
+
+- Assignee to ensure the resolution of the escalation is reflected in the PR
+  review.
+
+.. _#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 that the reviewer has requested blocking changes.
+    #. PRs assigned to the reviewer as the area maintainer.
+    #. All other PRs.
+
+- Try to provide feedback on the entire PR in one shot. This provides the
+  committer 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,
+  reviewers should add 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.
+
+- When using the "Request Changes" option, mark trivial, non-functional,
+  requests as "Non-blocking" in the 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
+
+.. [#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.

doc/contribute/guidelines.rst

@@ -12,6 +12,8 @@ This document explains how to participate in project conversations, log bugs
 and enhancement requests, and submit patches to the project so your patch will
 be accepted quickly in the codebase.
 
+.. _licensing_requirements:
+
 Licensing
 *********
 
@@ -816,25 +818,8 @@ in the Git commit's ``Author:`` field.
 Other Commit Expectations
 =========================
 
-* Commits must build cleanly when applied on top of each other, thus avoiding
-  breaking bisectability.
-
-* Commits must pass all CI checks (see `Continuous Integration`_ for more
-  information)
-
-* Each commit must address a single identifiable issue and must be
-  logically self-contained. Unrelated changes should be submitted as
-  separate commits.
-
-* You may submit pull request RFCs (requests for comments) to send work
-  proposals, progress snapshots of your work, or to get early feedback on
-  features or changes that will affect multiple areas in the code base.
-
-* When major new functionality is added, tests for the new functionality MUST be
-  added to the automated test suite. All new APIs MUST be documented and tested
-  and tests MUST cover at least 80% of the added functionality using the code
-  coverage tool and reporting provided by the project.
-
+See the :ref:`committer-expectations` for a more complete discussion of
+committer and reviewer expectations.
 
 Submitting Proposals
 ====================

doc/contribute/index.rst

@@ -7,6 +7,7 @@ Contributing to Zephyr
    :maxdepth: 1
 
    guidelines.rst
+   committer_expectations.rst
    coding_guidelines/index.rst
    documentation/index.rst
    external.rst

doc/project/project_roles.rst

@@ -108,7 +108,7 @@ in addition to those listed for Contributors and Collaborators:
   of involvement.
 * Right to make decisions in the relevant subsystems or areas of involvement,
   in conjunction with the collaborators and submitters.
-  See :ref:`escalation-process`.
+  See :ref:`pr_technical_escalation`.
 * Responsibility to convey the direction of the relevant subsystem or areas to
   the TSC
 * Responsibility to ensure all contributions of the project have been reviewed
@@ -154,7 +154,9 @@ the latter is not possible.
   of the code
 * Responsibility to drive the pull request to a mergeable state
 * Solicit approvals from maintainers of the subsystems affected
-* Responsibility to drive the escalation process
+* Responsibility to drive the :ref:`pr_technical_escalation` process
+
+.. _release-engineering-team:
 
 Release Engineering Team
 ++++++++++++++++++++++++
@@ -305,6 +307,8 @@ Release Activity
          :align: center
          :alt: Release Activity
 
+.. _merge_criteria:
+
 Merge Criteria
 ++++++++++++++
 
@@ -341,27 +345,3 @@ Merge Criteria
 * A minimum review period of 2 days, 4 hours for trivial changes (see
   :ref:`review_time`). Hotfixes can be merged at any time after CI passes.
 * All required checks are passing
-
-.. _escalation-process:
-
-Escalation Process
-++++++++++++++++++
-
-* Contributors may object to change requests or decisions made by
-  Maintainers.
-* Process
-
-  * Resolve in the PR among assignee, maintainers and reviewer
-
-    * Assignee to act as moderator if applicable
-
-  * Optionally resolve in the dev review meeting with more Maintainers
-    and project stakeholders
-
-    * The involved parties and the Assignee to be present when
-      the (escalated) issue is discussed
-
-  * TSC: Assignees can escalate to the TSC voting members and get
-    a binding resolution in the TSC.
-  * Assignee to ensure the resolution of the escalation is
-    reflected in the PR review.