Pull Request/Merge Request

You want a transparent mechanism for Code Reviews that facilitates Automated Checks, for a distributed team. This pattern describes an approach to facilitate code reviews that offer timely, relevant, and actionable feedback in a shared space, potentially including people who aren’t in the same room or can’t meet at the same time.

The Challenges of Remote Collaboration

When a change is ready for integration, you may want a Code Review from a team member. This should happen quickly to avoid delaying integration. A good Code Review process balances: - Speed: Timely feedback so as not to slow down integration more than necessary, - Quality of feedback: Relevant and actionable comments to improve the code. - Flow: To minimize the cost of delays for the author, and context switches for the reviewer.

The easiest way to get feedback is to ask someone nearby to look at your code or perhaps to invite someone who is available to view the code by screen sharing. The most available person may not be the person who can give the best feedback, and the person who can give the best feedback may not be the most available.

Allowing for asynchronous feedback can address the availability issue; asking someone to review and comment “as soon as they can” can allow for a productive context switch, assuming that the delay isn’t too long. Having time to prepare and comment thoughtfully can also improve quality, but handoffs and delays in conversation can negatively affect collaboration and productivity, You’d like to be able to get informed feedback ‘quickly enough’ without negatively impacting others on the team.

Scheduling times for review can minimize the cost of feedback as Slow Productivity [1] suggests:

“A direct strategy for reducing collaboration overhead is to replace asynchronous communication with real-time conversations. … Arranging these conversations, however, is tricky. There’s a reason why the saying this meeting could have been an email has entrenched itself as a workplace meme in recent years. If every task generates its own meeting, you’ll end up trading a crowded inbox for a calendar crowded with meetings—a fate that is arguably just as dire. … The right balance can be found in using office hours: regularly scheduled sessions for quick discussions that can be used to resolve many different issues.”

While scheduled collaboration is a common pattern in agile methods (the Daily Scrum, the Sprint Review, etc), tasks get done at unpredictable times. So, having one defined review time might be too few, but too many can lead to a lack of solid focus time.

If you offer reviewers time to prepare for feedback, one option is to have each person download the branch locally, build it, and review it, which adds complexity to the process and limits collaboration possibilities.

An alternative, using a centralized mechanism like GitHub, which allows for browsing and comments, can simplify the browsing workflow, but the ability to comment can lead to batched, asynchronous review work.

For remote or distributed teams, mechanisms that depend on real-time, synchronous collaboration can be challenging.

The Automated Checks that a Continuous Integration Build offers can provide important insights into the state of the code and allow humans to focus on higher-level issues. You can do a risk analysis of the code based on certain heuristics (or at the judgment of the developer) to skip a review for trivial changes, as long as the Automated Checks pass. Having a way to ensure that these checks run before the human review starts is valuable: A consistent process to run Automated Checks and Unit Tests in ant integration environment which can incorporate that feedback into the review provides a backstop for inadvertent errors, and provide an opportunity to catch errors before the code is merged.

Some regulated industries require some documentation of reviews, and you want to minimize the overhead of that documentation process. Unrelated to any compliance requirements, having data about the review process (timing, changes, etc.) might be useful for tuning the process and identifying process issues that you can address in a Retrospective.

You want to balance the speed, quality, and relevance of feedback.

Leverage Tools in an Adaptive Way

Establish a Pull Request Process that encourages collaboration and rapid feedback. Favor interactive reviews in a time frame that works for the team, but allows for asynchrony when it reduces communication overhead without hurting communication. By default, avoid blocking Pull Requests for all but the most significant changes; trust team members to address issues or to reach out for clarification.

A Pull Request is a form *Code Review * done in a shared environment. This enables:

• Easier collaboration across locations.
• Automated checks to be run, with feedback shared in a common place.
• A place for team members to share feedback and propose specific changes.
• When needed for compliance purposes, documentation of a review

The essential elements of a Pull Request are:

• A way to announce that code is “ready to merge into the Main Line.”
• A way to request feedback in the form of comments or proposed changes
• A way to respond to feedback through comments or code changes.
• A way for automated builds, and checks to run on the code before a change is integrated.

The following things are not essential elements of a Pull Request:

• Integration gated by approval: While approval can be part of the process, a sense of shared responsibility makes for a better delivery pipeline.
• Asynchronous comment threads: Asynchrony can be useful when there is a Time Zone difference between teams (for example, sharing end-of-day work for another team’s feedback might be useful), but the goal should be to integrate as quickly as possible.

While Pull Requests can be asynchronous, the comment process need not be; feedback can be collaborative, asynchronous, or a combination, depending on the team’s agreements. Pull Requests can be challenging when they introduce unnecessary delays. But not all delay is problematic, and some can be valuable.

Consider the life of a pull request:

1. A Pull Request is opened
2. Automated checks are run
3. Team members review the code
4. Review conversation happens
5. Author makes code changes
6. Code is merged

The “review conversation” can happen:

• Immediately when complete: this optimizes for feedback speed but doesn’t allow for reflection or context switching.
• Scheduled review times: Much like the Daily Scrum is a way for a Scrum team to have a focused time to discuss how to collaborate, agreeing on when reviews happen can reduce interruptions.
• Scheduled delay after code is complete. This is often a good balance between immediate and strictly scheduled, as it allows team members to transition from other work.

Even with scheduled times, the team can always agree to exceptions when needed and fall back to immediate review. Much like deciding between having a meeting and sending a message there are cases when having time to comment asynchronously before an interactive review can simplify and speed the feedback process. As Cal Newport writes [1] :

A direct strategy for reducing collaboration overhead is to replace asynchronous communication with real-time conversations. … Arranging these conversations, however, is tricky. There’s a reason why the saying this meeting could have been an email has entrenched itself as a workplace meme in recent years. If every task generates its own meeting, you’ll end up trading a crowded inbox for a calendar crowded with meetings—a fate that is arguably just as dire. … The right balance can be found in using office hours: regularly scheduled sessions for quick discussions that can be used to resolve many different issues.

The choice of approach depends on the team context. If a team is highly focused, “immediate” could work. “Scheduled delay” (“in 30 mins”) or even “every hour”) could be a challenge if team members find their time consumed with meetings outside the team, in which case reserving a block of time for everyone to do reviews might be the best choice, though excessive batching can introduce waste. A few times a day or “every two hours” can be a reasonable compromise.

Sometimes, you can combine the “Review code” and “Review conversation” steps.

If the team commits to a process, this review process can be complete in an hour or two, even allowing for a delay between “ready for review” and “first comment.” Any review process (with the exception of continuous code review with pairing) will inevitably have some delay as people shift contexts. You want to have a pull request process that minimizes the length of the “review conversation.” To do this:

• Avoid asynchronous comments threads. “Pre-review” comments are useful to give the team a chance to understand the code, but avoid long asynchronous dialogs:
  • If comments are minimal and clear, trust the author to address comments
  • If discussion is needed, arrange for an interactive discussion time.
• Don’t require a second round of reviews. After any discussion, trust the author to address comments appropriately.

A Pull Request can be a framework for interactive feedback that balances the value of interactivity and flow.

A Pull Request will be effective with certain practices in place:

• Working agreements about collaboration:
  • How quickly review should happen
  • Times when people will be available for collaborative reviews.
• Interactive Reviews:
  • After an initial review period, review changes together, ending with an action plan of what changes the developer will make before merging.
• Conditional Approvals for asynchronous comments.
  • When comments happen asynchronously, trust that the developer will address the concerns or ask for clarification. Save “Request Changes” for significant errors. And ask to meet.
• Limit the use of gates to things like:
  • Failed Tests
  • Failed Automated Checks that are known to indicate high-severity issues,
• Keeping the change sets small to minimize the other factors that slow down Code Review.

The overarching criteria is whether the Pull Requests makes it difficult for the team to meet its integration goal. If merging code “within a day” is the goal, this probably means a feedback cycle of approximately 2 hours.

Avoid making a Pull Request process too cumbersome because you believe that you work in an environment that requires that. Regulated environments allow for process automation and improvements [2] :

Readers who work in highly regulated environments might think they cannot just change a process on a whim, and it is easier to live with a broken process than go through the steps to fix it. But no regulatory body requires you to keep buggy processes. In fact, they usually have requirements that broken processes be fixed in a timely manner. Git PR provides a way to make a change with approvals, tracking, and accountability that enable you to comply with regulations more efficiently.

Which is not to say that you can still avoid all human review steps in some regulated industries, but you can work to streamline the process [3]:

To a large degree, automation can help to perform the compliance checks. However, the developer must be aware of them and their intended consequences, which must be documented. Furthermore, it is often a good practice to collect the change approval from the most qualified team members when change happens, as this may help in problematic cases.

Pull Requests are often associated with heavyweight processes involving gates and approvals. This isn’t necessary, but you can leverage the Pull Request Too to improve the quality of collaboration. Consider this flow: - The developer pushes their changes to the central repository and opens a Pull Request - The build pipeline runs a series of automated checks, including tests, style, and security checks. The style and security checks add annotations to the build. When the checks are complete, the build pipeline notifies the other team members that the code is available for feedback and suggests the team meet in 45 minutes. - Team members review the code and make comments. - If there are no substantial items to discuss: - The team members approve the Pull Request, and the meeting doesn’t happen. - Code is merged by the author after they review and address any feedback - If there are items to discuss: - The team meets at the appointed time and discusses comments. - After the meeting, the team approves the change - After the author addresses comments in the way that makes the most sense, they merge the change.

Cautions

Code Review in general, and Pull Requests in particular, are part of your collaboration dynamic, so it’s important to periodically check in on whether:

• The team is meeting its commitments to each other
• If the commitments are still valid

If you feel that a Pull Request processes take too long, consider gathering metrics, such as “time from open to close”, “time to first feedback,” and potentially “number of comments” to add some data to present at a retrospective. Also, consider impressions since a mismatch between data and feeling could indicate that the guidelines might need to be revisited.

All the criteria that make for a good Code Review will help you to avoid the major issue with Pull Requests, in particular ones that seem to drag out.

• A major problem with Pull Requests is a lag in starting (and finishing) reviews. This can be due to reviews not being a priority. Having the right Team Focus can mitigate this
• Feature Partners can help ensure that the reviews focus on the right things and are quick and that reviewers have the correct context.
• Automated Checks enable the humans on the process to higher level issues, leading syntax, static analysis, etc. tools to catch the things they are best at.
• Small Development Tasks can ensure that the code being reviewed is an appropriate size to be reviewed quickly and that the code can be merged independent of other changes (atomicity).

Some of the features of a Pull Request that make sense in the context of open source projects, such as required approvals and gating reviews, can slow down a team unnecessarily and lead to downstream costs. You don’t need to enable all of the gating features of a Pull Request if they don’t add value to your workflow.

While getting feedback is often helpful, and synchronous conversations can mitigate many risks related to delays, keep the review process aligned with risk level, and be wary of falling into a habit of synchronous feedback. Some changes might not need a formal review, and sometimes, asynchronous feedback makes sense. However , long comment exchanges can be a sign of deeper collaboration issues. Continually evaluate the process.

Notes

Consider making PR gates contingent on risk, perhaps using a tool like Shepherdly , which can assign a risk score to a change

The origins of the term PR in the context of Open Source are important to understand (and that the same toolchain is used in product and project settings, though with a different workflow.

References [bibliography]