-
Notifications
You must be signed in to change notification settings - Fork 1.7k
doc: GitHub PRs workflow #8920
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
doc: GitHub PRs workflow #8920
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,36 @@ | ||||||
| GitHub PRs workflow | ||||||
| =================== | ||||||
|
|
||||||
| Draft PRs | ||||||
| ~~~~~~~~~ | ||||||
|
|
||||||
| A 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 explicit 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` | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. requires `` to create inline code:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||||||
|
|
||||||
| A draft may be closed if it has not been updated in the last month. | ||||||
|
|
||||||
| Mergeable PRs | ||||||
| ~~~~~~~~~~~~~ | ||||||
|
|
||||||
| When a Pull Request is intended to be merged as is, the workflow is the following : | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space before the ":" :P |
||||||
| 1. get reviewed, and either request changes or get approved | ||||||
| 2. if approved, get staged in a next branch (with other PRs), wait for CI validation (and eventually request changes if CI finds anything) | ||||||
| 3. get merged and closed | ||||||
|
Comment on lines
+18
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These require indentation to be rendered as a list, with one item per line
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, adding a space at the beginning of each line, right ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I usually add a couple, to be safe. Their documentation doesn't mention, this, to be honest, but at least locally I always need that to prevent them from being rendered as simple inline text. https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html |
||||||
|
|
||||||
| 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 : 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` | ||||||
| Such a PR may be closed if it has not been updated in the last month. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed with the team, isn't a month too short, considering some folks take long vacations?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving to 2 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>`. | ||||||
|
|
||||||
| #TODO filter does not work cf https://github.com/OISF/suricata/pull/8898 | ||||||
| GitHub filter for approved PRs that should go in a next branch is `is:pr is:open draft:false sort:updated-asc review:approved` | ||||||
| Command to get approved PRs is `gh pr list --json number,reviewDecision --search "state:open type:pr -review:none" | jq '.[] | select(.reviewDecision=="")'` | ||||||
|
|
||||||
| Once in approved state, the PRs are in the responsibility of the merger, along with the next branches/PRs. | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,3 +6,4 @@ Contributing | |
|
|
||
| contribution-process | ||
| code-submission-process | ||
| github-pr-workflow | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to enforce a character limit around 79/80 per line.