Skip to content

Commit

Permalink
devguide: reorganize pr-workflow section
Browse files Browse the repository at this point in the history
This section seemed to aim both at PR reviewers and PR authors at the
same time, even though some info is probably of low value for
contributors.

Created new section for PR reviewers and maintainers, and kept the info
for PR authors separated. Also highlighted information on requested
changes and stale PRs.
  • Loading branch information
jufajardini committed Dec 14, 2023
1 parent d16b827 commit 2fd74af
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 23 deletions.
17 changes: 9 additions & 8 deletions doc/userguide/devguide/contributing/code-submission-process.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,17 @@ Commits
#. Commits need to be logically separated. Don't fix unrelated things in one commit.
#. Don't add unnecessary commits, if commit 2 fixes commit 1 merge them together (squash)
#. Commits need to have proper messages, explaining anything that is non-trivial
#. Commits should not at the same time change, rename and/or move code. Use separate commits
for each of this, e.g, a commit to rename files, then a commit to change the code.
#. Commits should not, at the same time, change, rename and/or move code. Use separate commits
for each of this, e.g, a commit to rename files, then a commit to change the code.
#. Documentation updates should be in their own commit (not mixed with code commits)
#. Commit messages need to be properly formatted:
* Meaningful and short (50 chars max) subject line followed by an empty line
* Naming convention: prefix message with sub-system ("rule parsing: fixing foobar"). If
you're not sure what to use, look at past commits to the file(s) in your PR.
* Description, wrapped at ~72 characters
#. Commit messages need to be properly formatted (check the example further
below in this section):
* Meaningful and short (50 chars max) subject line followed by an empty line
* Naming convention: prefix message with sub-system (**"rule parsing: fixing foobar"**). If
you're not sure what to use, look at past commits to the file(s) in your PR.
* Description, wrapped at ~72 characters
#. Commits should be individually compilable, starting with the oldest commit. Make sure that
each commit can be built if it and the preceding commits in the PR are used.
each commit can be built if it and the preceding commits in the PR are used.
#. Commits should be authored with the format: "FirstName LastName <name@example.com>"

Information that needs to be part of a commit (if applicable):
Expand Down
49 changes: 34 additions & 15 deletions doc/userguide/devguide/contributing/github-pr-workflow.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ GitHub Pull Request Workflow
Draft Pull Requests
~~~~~~~~~~~~~~~~~~~

A Pull Request (PR) should be marked as `draft` if it is not intended to be merged as is,
A Pull Request (PR) should be marked as *draft* if it is not intended to be merged as is,
but is waiting for some sort of feedback.
The author of the PR should be explicit with what kind of feedback is expected
(CI/QA run, discussion on the code, etc...)

GitHub filter is ``is:pr is:open draft:true sort:updated-asc``
The GitHub filter is ``is:pr is:open draft:true sort:updated-asc``.

A draft may be closed if it has not been updated in two months.

Expand All @@ -22,25 +22,44 @@ When a Pull Request is intended to be merged as is, the workflow is the followin
(and eventually request changes if CI finds anything)
3. get merged and closed

A newly created PR should match the filter
``is:pr is:open draft:false review:none sort:updated-asc no:assignee``
Once submitted, we aim at providing a first PR review within two weeks and a
month.

If either code, documentation wording or commit messages need re-work, the
reviewer will set the PR state to *changes requested*.

.. note:: It is expected that the author will create a new PR with a new version
of the patch as described in :ref:`Pull Requests Criteria <pull-requests-criteria>`.
A PR may be closed as stale if it has not been updated in two months after
changes were requested.

A PR may be labeled *decision-required* if the reviewer thinks the team needs
more time to analyze the best approach to a proposed solution or discussion
raised by the PR.

Once in approved state, the PRs are in the responsibility of the maintainer, along
with the next branches/PRs.

Reviewers and Maintainers
-------------------------

A newly created PR should match the filter::

is:pr is:open draft:false review:none sort:updated-asc no:assignee

The whole team is responsible to assign a PR to someone precise within 2 weeks.

When someone gets assigned a PR, the PR should get a review status within 2 weeks:
When someone gets assigned a PR, it should get a review status within 2 weeks:
either changes requested, approved, or assigned to someone else if more
expertise is needed.

GitHub filter for changes-requested PRs is ``is:pr is:open draft:false sort:
updated-asc review:changes-requested``
The GitHub filter for changes-requested PRs is::

Such a PR may be closed if it has not been updated in two months.
It is expected that the author creates a new PR with a new version of the patch
as described in :ref:`Pull Requests Criteria <pull-requests-criteria>`.
is:pr is:open draft:false sort: updated-asc review:changes-requested

Command to get approved PRs is ``gh pr list --json number,reviewDecision --search
"state:open type:pr -review:none" | jq '.[] | select(.reviewDecision=="")'``
The command to get approved PRs is::

Web UI filter does not work cf https://github.com/orgs/community/discussions/55826
gh pr list --json number,reviewDecision --search "state:open type:pr -review:none" | jq '.[] | select(.reviewDecision=="")'

An approved PR should match the filter: ``is:open is:pr review:approved``.

Once in approved state, the PRs are in the responsibility of the merger, along
with the next branches/PRs.

0 comments on commit 2fd74af

Please sign in to comment.