 Please ask about problems and questions regarding this tutorial on answers.ros.org. Don't forget to include in your question the link to this page, the versions of your OS & ROS, and also add appropriate tags. |
ROS-Industrial Pull Request Review Process
Description: Outline of the ROS-Industrial core repository pull request review process.Keywords: industrial commit process review quality second pair of eyes
Tutorial Level: ADVANCED
Overview
This page outlines the peer review process for pull requests and commits to the core repositories of the ROS-Industrial GitHub organisation. It provides a rationale for why this protocol is put in place, and what the project intends to gain by following it.
Rationale
With the core ROS-Industrial repositories reaching a certain level of maturity, the focus of development has shifted from implementing new features to bug-fixing and general maintenance. Automated systems checking for regressions have been put in place to guard against unintended effects of developer changes, and unit tests are employed throughout the codebase.
However, these automated systems are not infallible and not all checks can be automated. In those cases, a review of proposed changes by other developers can provide the necessary input. In essence, other developers then provide additional pairs of eyes, increasing the chances of spotting bugs, and guarding against unwanted deviations from design [1].
The practice of code reviews has been common place within ROS, ever since its creation. The OSRF has implemented a similar policy as the one described on this page. The ROS-Industrial organisation aims to improve its development process by requiring commit and pull request reviews of all commits and pull requests against any of the core repositories.
Process
Primary rule:
Pull requests will not be merged unless they have the approval of at least one (additional) ROS-Industrial committer.
Reviewers do not have to be core developers of the repository targeted by the pull request, although in practice they probably almost always will be (as they have the deepest understanding of the code and are most capable of estimating the impact of a particular change).
Secondary rule:
Pull requests must be small. Each pull request should represent an incremental change or improvement. Even large feature additions can be broken down into small changes. At most a pull request should be about a week's worth of effort and/or single class/unit test. Large pull requests will be rejected.
Outline
Submit a pull request against the target repository/branch1
Use the description field to mention the name of any particular developer that you'd want to take a look. Optionally, use a comment on the pull request to do this. Use the @account_name syntax for this.
Other developers (possibly the mentioned one(s)) review the proposed changes. Any comments can either be posted as a single comment on the pull request, or in-line with the code2.
If the reviewer is ok with the proposed changes and is satisfied that all issues raised (by other reviewers) have been addressed, he or she approves the pull request by commenting on it with a "+1" or "lgtm" (looks good to me).
- After the pull request has been approved, the reviewer or another developer with commit access can proceed to merge the pull request.
See also GitHubs How to write the perfect pull request.
Repositories affected
Initially, the described review process only applies to the core ROS-Industrial repositories, ie those part of the ros-industrial and ros-industrial-consortium organisations.
Contact
Remarks, comments and questions can be directed to the ROS-Industrial category on ROS Discourse.
Notes
Make sure your source branch is up-to-date with changes in the destination branch before submitting the request. If you haven't made any changes to your branch yet, you can use a git pull <url> <branch_name>. If you have made changes, try to rebase onto the destination branch first. (1)
Depending on tooling support and setup, build-tests would be automated and do not fall under the responsibility of the reviewer (2)
References
1. Raymond, Eric S., The Cathedral and the Bazaar, 2002-08-02 09:02:14, online, retrieved 2013-11-14
