Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add some best practices to streamline PR approval process #468

Open
sbmetz opened this issue Apr 10, 2024 · 5 comments
Open

add some best practices to streamline PR approval process #468

sbmetz opened this issue Apr 10, 2024 · 5 comments
Labels
General update General updates to the guide or repo

Comments

@sbmetz
Copy link
Contributor

sbmetz commented Apr 10, 2024

[Background] Since we require 3 approvals to merge an SSG change, any changes that happen after one or two approvals invalidate those approvals, effectively resetting the "clock" to 0. This can mean that early approvers may be asked to "re-approve" as additional changes are made. In general, this is appropriate -- even small late-breaking changes can be meaningful to the whole pull request. General consensus at the March 2024 style guide meeting was to not change the approval process, but we agreed that the re-approval churn can be distracting.

I am opening this issue to propose some guidelines/best practices to the admin responsibilities page.

Here are some suggestions -- please expand, add other ideas, upvote/downvote an idea (that is, chime in on what you like or don't like), suggest alternate wording, etc. The idea isn't to be overly prescriptive (there are many paths to effective collaboration), but to offer suggestions that might streamline the approval process.

  • Review the issues and existing discussion, especially around complex pull requests (several files or several edits). If the status of the PR is unclear, ask for clarity before asking for approvals. This step still drives the PR toward completion.

  • If there are multiple complex pull requests pending, consider staggering the requests for approvals. When several outstanding PRs are candidates for approval, and reviews require changes and re-approvals, it can be hard for project members to keep up with the slack channel traffic.

  • On the other hand, grouping simple pull requests into a batch approval request might help approvers maximize their repository visit.

@bredamc
Copy link
Contributor

bredamc commented Apr 11, 2024

Great ideas! Thanks, @sbmetz 👍

@bredamc
Copy link
Contributor

bredamc commented Apr 11, 2024

Another suggestion: Create a separate Slack channel for PR approval requests, to target only those who can actually approve. Perhaps add issue discussions in that channel too.

@bergerhoffer bergerhoffer added the General update General updates to the guide or repo label Apr 11, 2024
@bergerhoffer
Copy link
Collaborator

Posted in slack, but I'd prefer not to create a separate slack channel for approvals. Other folks in the channel might actually be interested in seeing what's coming (and might want to weigh in on the discussion). We have the @ ccs-style-council slack alias for reaching just those on council who can do approvals, if needed.

Plus also that I have too many slack channels already 😅. And I don't think there is too much activity in that channel that another channel is necessary. There are usually only a handful of threads each day, if any.

@snarayan-redhat
Copy link

@bergerhoffer do we have a link to the slack thread? It might be easier to keep a track of where the issue stands.

@sbmetz
Copy link
Contributor Author

sbmetz commented Jun 5, 2024

Here's the related slack thread: https://redhat-internal.slack.com/archives/C04HKR61MN1/p1712829051295799

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General update General updates to the guide or repo
Projects
None yet
Development

No branches or pull requests

4 participants