1 min | Scribe | All | @Craig McNally is next, followed by @Tod Olson Reminder: Please copy/paste the Zoom chat into the notes. If you miss it, this is saved along with the meeting recording, but having it here has benefits. |
* | PR Templates | All | https://folio-project.slack.com/archives/CAQ7L02PP/p1713445649504769 Hello team, Small request to consider.
Regarding pr templates. What I suggest is that, pr template shouldn't be any instructions, because most developer who are creating pr have already understand the rules. If we put just two section into template, it will encourage developers to write more about their work and that lead to knowledge sharing among developers.
Notes: 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 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. 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 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.
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
|
| Zoom Chat |
| 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. |