2024-06-26 - PR Templates

https://folio-project.slack.com/archives/CAQ7L02PP/p1713445649504769

1. From my perspective, pr template is not good idea. Even the biggest open source projects that are contributed by many people don't have any pr template. Currently what we have for acq modules https://github.com/folio-org/mod-orders-storage/blob/master/PULL_REQUEST_TEMPLATE.md
2. These pr template is inconsistent in different teams.

• A lot of us have come full circle in initially supporting PR templates, but have since changed our minds after seeing how they've been (mis)used.
• The idea of templates is good, but how they're being used is more problematic than helpful
• They way they're being used today, there's a lot of misuse/misinformation
• Maybe there are possibilities to enforce PR hygiene via bots/automation
  • e.g. check if the PR links to a JIRA, etc.
• PR template check lists are often not honored and are just noise/distraction.  These should probably be removed
• It may be helpful for us to agree on what to do with the checklists - just remove them?  Simplify them?  link to some wiki page w/ checklists?
• At the heart of this I think we're trying to avoid blank/empty PRs... It probably would be helpful to identify and agree to where we should "set the bar" wrt the amount and types of information should ALWAYS be included in PRs.
  • A link to relevant Jira(s)
  • Purpose
  • ...
• PR templates are not entirely consistent across all repositories
• Do we want a centralized decision here, or leave it up to the individual teams? 
• We need to address the cultural aspect of this...  
  • E.g. if asked to review a PR, and it's missing key information, request the owner to change it before giving approval
    • Attempts at this in the past have not been effective
    • It would be helpful if we had well written PRs we could point to as examples.
  • PRs serve as documentation well after it's approved/merged/etc.  It's really helpful for determining which module release a particular change made it into.
    • putting the Jira key in the commit message would make this even easier, but that's not done consistently
• How often are PRs reviewed by those outside of the team?
  • On the UI, fairly often.  There's a rotation for PR review responsibilities among some UI devs to help teams who require 2 approvals, but only have 1 FE dev.
  • Without having external PR reviews, it's difficult to enforce PR rules, but having every PR reviewed by someone outside of the team is not tenable.
• Zak Burke will look for "good" PRs to use as examples

Today

Zak Burke  to  Everyone 11:14 AM
Example PR with an empty template: https://github.com/folio-org/ui-users/pull/2709

Marc Johnson  to  Everyone 11:22 AM
IIRC the build doesn’t stop a change that doesn’t meet the quality from going through

Is that right?

Tod Olson 11:23 AM
I believe that's right, it does not prevent the merge but does give a report on the state of the module.

You  to  Everyone 11:23 AM
Didn't we agree to PoC the use of a "Breaking Change" label on PRs?

You  to  Everyone 11:23 AM
I forget where that stands, but it's worth noting in this conversation

Marc Johnson 11:29 AM
I shared the labels with the TC

I don’t think the process or cultural change has started

You  to  Everyone 11:33 AM
That's one way to reduce how many PRs people ask you to review ;)

Zak Burke  to  Everyone 11:43 AM
PR guidelines I wrote a long time ago but never merged.

You  to  Everyone 11:44 AM
I'm thinking that maybe it's worth taking both a top-down and bottom-up approach here.... Engage member organizations, convincing them of the value/benefits, and letting them hand the message down to their developers.  Then, also encourage reviewers to push back on poorly crafted PRs

Tod Olson  to  Everyone 11:50 AM
I need to drop a little early. Agreed, culture change is a real challenge. Nice Google engineering reference. 
Thank you everyone.