Skip to content

Commit 4524335

Browse files
Update documentation on force-push policy during review
Add guidance in documentation to encourage contributors to avoid force pushing to a PR undergoing review. This is to ensure that the integrity of the comment history is preserved once review has started.
1 parent d5136b6 commit 4524335

File tree

1 file changed

+5
-4
lines changed

1 file changed

+5
-4
lines changed

docs/contributing/guidelines/workflow.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,8 @@ Pull requests on GitHub have to meet the following requirements to keep the code
4949
- Because we use GitHub, special commit tags that other projects may use, such as “Reviewed-by”, or “Signed-off-by”, are redundant and should be omitted. GitHub tracks who reviewed what and when.
5050
- Prefixing your commit message with a domain is acceptable, and we recommend doing so when it makes sense. However, prefixing the domain with the name of the repo is not useful. For example, making a commit entitled "mbed-drivers: Fix doppelwidget frobulation" to the `mbed-drivers` repo is not acceptable because it is already understood that the commit applies to `mbed-drivers`. Renaming the commit to "doppelwidget: Fix frobulation" would be better, if we presume that "doppelwidget" is a meaningful domain for changes, because it communicates that the change applies to the doppelwidget area of `mbed-drivers`.
5151
- All new features and enhancements require documentation, tests and user guides for us to accept them. Please link each pull request to all relevant documentation and test pull requests.
52-
- Avoid merging commmits. (Always rebase when possible.)
52+
- Avoid merging commits. (Always rebase when possible.)
53+
- Avoid force pushing when making review changes, unless you're cleaning up your branch's history once the changes have been approved.
5354
- Comment in the pull request on every change (rebase or new commits). This helps reviewers to be up to date with changes
5455
- Pull requests should fix a bug, add a feature or refactor.
5556
The only exceptions are third-party version updates (for example, Mbed TLS or Nanostack releases for Mbed OS). These updates should provide Mbed OS release notes in the pull request description, or link to an external changelog or release notes.
@@ -193,7 +194,7 @@ Each pull request goes through the following workflow:
193194

194195
## Pull request states
195196

196-
Mergify bot drives our workflow. The mergify rules are defined in the Mbed OS repository in the .mergify.yml file. The Mbed OS maintainers are responsible for moving pull requests through the workflow states with help from the mergify bot.
197+
Mergify bot drives our workflow. Its settings are defined in the file [`mergify.yml`](https://github.com/ARMmbed/mbed-os/blob/master/.mergify.yml). The Mbed OS maintainers are responsible for moving pull requests through the workflow states with help from the mergify bot.
197198

198199
Each state is time-boxed. In most cases, sufficient time is provided to move to another state. The ciarmcom bot periodically checks that pending activities on pull requests are completed in a timely manner.
199200
<center>
@@ -216,10 +217,10 @@ Pull requests are closed if they are idle for more than two weeks. The author or
216217

217218
All pull requests must be reviewed. The Arm Mbed CI bot determines the most suitable person to review the pull request (based on the files changed) and tags that person accordingly. A PR creator can request specific reviewers by @ tagging people or teams in the *Reviewers* section of the pull request template. For example, @personA @TeamB.
218219

219-
Mergify dismisses a reviewer's status after any change to the pull request commit history (such as adding a new commit or rebasing). Smaller changes, such as documentation edits or rebases on top of latest master, only require additional review by maintainers. Their approval is sufficient because a team assigned as a reviewer already approved the pull request.
220+
Mergify dismisses a reviewer's status after any change to the pull request commit history (such as adding a new commit or rebasing). As mentioned above, it is important that you not force push to your branch when your PR is undergoing review. This is discouraged because it may cause the reviewers comment history to be lost, and have other unintended effects. Smaller changes, such as documentation edits or rebases on top of latest master, only require additional review by maintainers. Their approval is sufficient because a team assigned as a reviewer already approved the pull request.
220221

221222
- Label: `needs: review`.
222-
- Time: Three days for reviewers to leave feedback after the autoreviewer bot has added the label.
223+
- Time: Three days for reviewers to leave feedback after the auto-reviewer bot has added the label.
223224

224225
### The Continuous Integration (CI) testing
225226

0 commit comments

Comments
 (0)