From 5d67ef7040d6ea8f715c3b1d2b54924311c3a6f8 Mon Sep 17 00:00:00 2001 From: Keith Short Date: Thu, 1 Dec 2022 16:16:34 -0700 Subject: [PATCH] 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 --- doc/contribute/committer_expectations.rst | 248 ++++++++++++++++++++++ doc/contribute/guidelines.rst | 12 +- doc/contribute/index.rst | 1 + doc/project/project_roles.rst | 4 + 4 files changed, 261 insertions(+), 4 deletions(-) create mode 100644 doc/contribute/committer_expectations.rst diff --git a/doc/contribute/committer_expectations.rst b/doc/contribute/committer_expectations.rst new file mode 100644 index 000000000000000..eecee844405f353 --- /dev/null +++ b/doc/contribute/committer_expectations.rst @@ -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 ` [#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 ` +describing the full scope of change and future work. The :ref:`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 :ref:`RFC proposal ` include: + +- Submitting a new feature. +- Submitting a new API. +- :ref:`treewide-changes`. +- Other large changes that can benefit from the :ref:`RFC proposal ` + 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`. + +- 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 `. + +.. _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 ` associated with the PR. Instead add + your suggestions as a comment to the :ref:`RFC `. 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. diff --git a/doc/contribute/guidelines.rst b/doc/contribute/guidelines.rst index 3b1b0129aa59b10..5a88e2596557d57 100644 --- a/doc/contribute/guidelines.rst +++ b/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 ********* @@ -560,8 +562,8 @@ workflow here: The ``-s`` option automatically adds your ``Signed-off-by:`` to your commit message. Your commit will be rejected without this line that indicates your - agreement with the `DCO`_. See the `Commit Guidelines`_ section for - specific guidelines for writing your commit messages. + agreement with the `DCO`_. See the :ref:`commit_message_guidelines` section + for specific guidelines for writing your commit messages. #. Push your topic branch with your changes to your fork in your personal GitHub account:: @@ -641,8 +643,10 @@ workflow here: `Continuous Integration`_. -Commit Guidelines -***************** +.. _commit_message_guidelines: + +Commit Message Guidelines +************************* Changes are submitted as Git commits. Each commit message must contain: diff --git a/doc/contribute/index.rst b/doc/contribute/index.rst index 36a3e44df0685ee..b40ca051f23e741 100644 --- a/doc/contribute/index.rst +++ b/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 diff --git a/doc/project/project_roles.rst b/doc/project/project_roles.rst index 6389f707c61c8a2..a694dd392542248 100644 --- a/doc/project/project_roles.rst +++ b/doc/project/project_roles.rst @@ -155,6 +155,8 @@ the latter is not possible. * Solicit approvals from maintainers of the subsystems affected * Responsibility to drive the escalation process +.. _release-engineering-team: + Release Engineering Team ++++++++++++++++++++++++ @@ -304,6 +306,8 @@ Release Activity :align: center :alt: Release Activity +.. _merge_criteria: + Merge Criteria ++++++++++++++