Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Add Committer Expectations document #52731

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
280 changes: 280 additions & 0 deletions doc/contribute/contributor_expectations.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,280 @@
.. _contributor-expectations:

Contributor Expectations
########################

Overview
********

The Zephyr project encourages :ref:`contributors <contributor>` 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 blocks of time to 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.

keith-zephyr marked this conversation as resolved.
Show resolved Hide resolved
.. note::
This page does not apply to draft PRs which can have any size, any number of
commits and any combination of smaller PRs for testing and preview purposes.
Draft PRs have no review expectation and PRs created as drafts from the start
do not notify anyone by default.


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 a functional code change.

- 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
===============================

Contributors 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, contributors 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 that contributors 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.
Contributors 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.

- :zephyr_file:`Kernel timer tests <tests/kernel/timer/timer_behavior>`
provide around 85% test coverage for the
:zephyr_file:`kernel timer <kernel/timer.c>`, measured by lines of code.
- Emulators for off-chip peripherals are an effective way to test driver
APIs. The :zephyr_file:`fuel gauge tests <tests/drivers/fuel_gauge/sbs_gauge>`
use the :zephyr_file:`smart battery emulator <drivers/sensor/sbs_gauge/emul_sbs_gauge.c>`,
providing test coverage for the
:zephyr_file:`fuel gauge API <include/zephyr/drivers/fuel_gauge.h>`
and the :zephyr_file:`smart battery driver <drivers/fuel_gauge/sbs_gauge/sbs_gauge.c>`.
- Code coverage reports for the Zephyr project are available on `Codecov`_.

- 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.

- Changes to APIs must increment the API version number according to the API
version rules.

- 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 that contributors break up a PR into smaller PRs and may
request that they create an :ref:`RFC proposal <rfcs>`.

.. _`Codecov`: https://app.codecov.io/gh/zephyrproject-rtos/zephyr

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 responding to multiple comments and
keith-zephyr marked this conversation as resolved.
Show resolved Hide resolved
publishing the responses in bulk. This reduces the number of emails sent to
reviewers.

- As GitHub does not implement |git range-diff|_, try to minimize rebases in the
middle of a review. If a rebase is required, push this as a separate update
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As GitHub does not implement |git range-diff|_, try to minimize rebases in the middle of a review.

Could an admin please disable the "Update branch" button that shows up in every PR?

It's apparently just one checkbox:
https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-suggestions-to-update-pull-request-branches

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could an admin please disable the "Update branch" button that shows up in every PR?

Done

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.

.. |git range-diff| replace:: ``git range-diff``
.. _`git range-diff`: https://git-scm.com/docs/git-range-diff

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.

Contributors 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 contributor should request
help on Discord or send an email to the `Zephyr devel mailing list`_.

Note that for new PRs, contributors 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 contributor 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
contributor 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. PR authors should acknowledge every
review comment in some way, even if it's just with an emoticon.

.. _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
23 changes: 4 additions & 19 deletions doc/contribute/guidelines.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
*********

Expand Down Expand Up @@ -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:`contributor-expectations` for a more complete discussion of
contributor and reviewer expectations.

Submitting Proposals
====================
Expand Down
2 changes: 2 additions & 0 deletions doc/contribute/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ Contributing to Zephyr
:maxdepth: 1

guidelines.rst
contributor_expectations.rst
proposals_and_rfcs.rst
coding_guidelines/index.rst
documentation/index.rst
external.rst
Expand Down
55 changes: 55 additions & 0 deletions doc/contribute/proposals_and_rfcs.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
.. _rfcs:

Proposals and RFCs
##################

Many changes, including bug fixes and documentation improvements can be
implemented and reviewed via the normal GitHub pull request workflow.

Many changes however are "substantial" and need to go through a
design process and produce a consensus among the project stakeholders.

The "RFC" (request for comments) process is intended to provide a consistent and
controlled path for new features to enter the project.

Contributors and project stakeholders should consider using this process if
they intend to make "substantial" changes to Zephyr or its documentation. Some
examples that would benefit from an RFC are:

- A new feature that creates new API surface area, and would require a feature
flag if introduced.
- The modification of an existing stable API.
- The removal of features that already shipped as part of Zephyr.
- The introduction of new idiomatic usage or conventions, even if they do not
include code changes to Zephyr itself.

The RFC process is a great opportunity to get more eyeballs on proposals coming
from contributors before it becomes a part of Zephyr. Quite often, even
proposals that seem "obvious" can be significantly improved once a wider group
of interested people have a chance to weigh in.

The RFC process can also be helpful to encourage discussions about a proposed
feature as it is being designed, and incorporate important constraints into the
design while it's easier to change, before the design has been fully
implemented.

Some changes do not require an RFC:

- Rephrasing, reorganizing or refactoring
- Addition or removal of warnings
- Addition of new boards, SoCs or drivers to existing subsystems
- ...

The process in itself consists in creating a GitHub issue with the :ref:`RFC
label <gh_labels>` that documents the proposal thoroughly. There is an `RFC
template`_ included in the main Zephyr GitHub repository that serves as a
guideline to write a new RFC.

As with Pull Requests, RFCs might require discussion in the context of one of
the `Zephyr meetings`_ in order to move it forward in cases where there is
either disagreement or not enough voiced opinions in order to proceed. Make sure
to either label it appropriately or include it in the corresponding GitHub
project in order for it to be examined during the next meeting.

.. _`RFC template`: https://github.com/zephyrproject-rtos/zephyr/blob/main/.github/ISSUE_TEMPLATE/rfc-proposal.md
.. _`Zephyr meetings`: https://github.com/zephyrproject-rtos/zephyr/wiki/Zephyr-Committee-and-Working-Group-Meetings
Loading