From 2fd74afcc71858d3edf9ed0f025a9fe75c430a39 Mon Sep 17 00:00:00 2001 From: Juliana Fajardini Date: Wed, 22 Nov 2023 12:52:59 -0300 Subject: [PATCH] devguide: reorganize pr-workflow section 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. --- .../contributing/code-submission-process.rst | 17 ++++--- .../contributing/github-pr-workflow.rst | 49 +++++++++++++------ 2 files changed, 43 insertions(+), 23 deletions(-) diff --git a/doc/userguide/devguide/contributing/code-submission-process.rst b/doc/userguide/devguide/contributing/code-submission-process.rst index 22bf16046a40..99ac93d5e201 100644 --- a/doc/userguide/devguide/contributing/code-submission-process.rst +++ b/doc/userguide/devguide/contributing/code-submission-process.rst @@ -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 " Information that needs to be part of a commit (if applicable): diff --git a/doc/userguide/devguide/contributing/github-pr-workflow.rst b/doc/userguide/devguide/contributing/github-pr-workflow.rst index 618c966c43c2..51ed0568c05d 100644 --- a/doc/userguide/devguide/contributing/github-pr-workflow.rst +++ b/doc/userguide/devguide/contributing/github-pr-workflow.rst @@ -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. @@ -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 `. + 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 `. + 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.