Community Contributed Pull Requests Overview
Community contributed pull requests are also known as “OSPR”s (suggestions for other names welcome!). OSPR is a shorthand term for all pull requests issued against the `openedx` GitHub organization that are not generated from the tCRIL or 2U teams. These PRs need an amount of management:
Authors may need help resolving CLA issues
PRs need to be monitored to make sure they are still active and participants are responding
Assistance may be needed in finding reviewers within a particular repository who have the knowledge to give feedback
Authors should get a welcome message from human because that is more personable and people enjoy human-human interactions.
OSPR Project Manager Community Position
There is a need for one or more people in the community to help out with OSPRs, in a process we call “OSPR Triage.” This position will be known as the “OSPR Project Manager” position. These people would need to take at least 10-20 hours per month following up on old OSPRs, helping new OSPRs get connected to the right resources, and generally fielding questions. This is a slightly technical position, but can be done by someone who is not a coder.
OSPR Project Manager Access Rights
The Project Manager will need Triage Permissions on all repositories that are a part of the Open edX Platform, as the position requires the following abilities:
Assign and remove labels
Assign and remove people from PRs
Close and re-open PRs (this is an easy way to trigger tests, and stale PRs need to be closed from time to time)
Request PR reviews.
Given the above, this will require all OSPR Project Managers to first apply for Core Contributor rights via the standard process.
Specific Responsibilities
What follows is a set of questions and answers to situations the OSPR Project Manager may face. There will certainly be situations that arise that are not answered here. Questions should go to the tCRIL team.
Status Alerts / Prompt Descriptions
“This branch out-of-date with the base branch”, “This branch has conflicts that must be resolved”
These are the responsibility of the PR author to correct
Things to know (if you want, you can help on the PR):
Out of date = They need to rebase in this case; this is why it’s important to get things merged as the master tends to change
Conflict = Someone changed information on the same file; the author has to reconcile those changes
“Review Required. At least 1 approving review is required by reviewers with write access”
This means that a person with write access to the repo must provide a review before merging. All OSPRs require a person with write access to approve, regardless of this message showing or not. In most cases, the OSPR author does not have write access to the repo so the whole point of the Community Contribution Manager’s job is to get someone with write access to review & merge. In the case where an author (always a Core Contributor) does have write access to the repo, they still need to find another person with write access to approve their PR. No PR can be approved by the author - it always requires review by another person (who can merge to the repo).
“1 review requesting changes by reviewers with write access.”
Review notes can be found under the “files changed” tab; this prompt means the PR has been reviewed and now the author has to address feedback that was provided by the reviewer.
The reviewer who requested changes must provide a new review and indicate their requested changes are resolved before the PR can be merged.
Checks and Follow-Up
Although it’s not strictly required (cf. note below), it can be a good idea to explicitly ask if a PR is ready for review:
If you directly address the author, that might make it easier to get relevant updates about the PR’s status from them going forward (as they will know to ping you, and you can to filter GitHub notifications based on that).
Some companies put their PRs through an internal review process first. So even if a PR looks ready the author might not be ready for a maintainer or CC to review their changes.
The OSPR bot’s initial responses to new PRs includes notes like:
Please let us know once your PR is ready for our review and all tests are green.
This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.
When should we close an old pull request?
If PR has been open for a long time (>30 days) and there’s been no action on the pull request, AND the pull request is waiting on the author NOT a reviewer, @ the author and ask if it should stay open.
After two weeks with no additional response, close the PR with a kind message (eg, “Thank you for your contribution! We’re closing this at this time because it is stale; you may reopen this pull request, or open a new one, when you have time to come back to this work.”)
If a pull request has a 2U author and a 2U repo owner, OK for them to stay open unless they’re open for a long time (~30 days), in which case, OK to check on status. You will be able to distinguish a PR designated as an “OSPR” because it will have the
open-source-contributionlabel on it:
PRs generated by 2U and tCRIL engineers do not have this label.
Checks “not complete” vs. “failed”? Who looks into failed checks?
For contributors on a new repo (even if they’ve committed in a different repo) their PR needs to have approval before checks are run (since the tests execute code).
Failed checks are authors responsibility except for needing to complete a CLA (pull in Feanil/Michelle)
If a CLA check doesn’t complete and shows as “pending” for more than a few minutes, it is likely stuck (it does not mean the check failed). A rebase should fix the problem.
If a PR is still in draft state, there is no need to ping authors about failing checks.
If a PR is not in draft state and the author asks for review, it usually makes sense to ask them to fix failing checks first.
However, if a PR introduces a large, impactful set of changes that requires input from more than one team (e.g. product and engineering), it can be a good idea to get the review process started early, regardless of failing checks. We want to avoid scenarios where authors spend days fixing small issues and getting the build green, only to be told by product and/or engineering that their changes are not going to be accepted or have already been introduced by another PR.
When to ping on WIP/draft PRs?
Best practice is if something has been WIP/draft for ~30 days
What if a PR has both checks failing AND is a WIP?
As mentioned above, failing checks don’t matter in a WIP as this doesn’t change code
Whenever a PR is a WIP, it should be in draft state (OK to ask author to convert it to draft if it isn’t already)
We generally take draft to mean that the author of a PR doesn’t consider the changes ready for review yet. If a PR is waiting for first checks or has some failing checks, that doesn’t mean that it should be marked as draft.
What if CLA is missing from 2U/Arbisoft/Lahore staff?
Alert them that they need to follow the procedure for new employees. Their manager (or 2U IT) needs to go through the process of creating a tCRIL ticket to register them in the system.
Their team must know what this means; this is not your job to walk them through a process people in their org know!
All checks passed; no conflicts = OK to merge?
Yes, but only someone with write access can perform the merge. You may need to remind the reviewer who gives the approval that they will need to perform the merge.
Note that this step is time sensitive. If a PR sits around too long before merging (for weeks or, in the case of repos with lots of activity, even just a few days) it may need to be rebased again, increasing effort for both the author and – sometimes at least – their reviewers.
Useful Links
External Contributions GitHub Board:
https://github.com/orgs/openedx/projects/19/views/1
Tickets not authored by 2U or tCRIL engineers go to this board when they come in
Once added to the board, it needs to get reviewed and merged
Use the board’s columns to change state of PR (can also change on the right side of the PR pages). The following guidelines should help determine appropriate status:
No Status is all PRs without a manually-assigned status. This is automatic grouping by the board, so all new OSPRs will go in there first. From there you pick them up and assign status.
Not ready: PR is still WIP/draft, or author is just not ready for whatever reason except CLA.
Ready for Review:
At a minimum the PR should not be a draft and should have a green CLA check.
Unless the PR introduces a large, impactful set of changes that requires input from more than one team (cf. above) all checks should be green/passing on it. (The idea is that we don’t want to waste reviewers' time on PRs that are half-baked – if checks are failing there is a chance that the author never resolves the issues and the PR ends up getting closed instead of merged.)
The author asked for review or confirmed that the PR is ready for review when prompted.
In Review: self-explanatory.
Blocked:
PRs with missing CLAs (these might eventually get a separate column on the board)
PRs that are blocked by other work (e.g. PRs that need to be merged after some other PR or event, PRs that have been deprioritized by their authors in favor of other work, PRs that are waiting for funding or client input, etc.)
Done: PR has been merged or closed.
Core Contributors and their repos:Core Contributors to the Open edX Project
CCs on a repo can provide an approval and merge a PR
Many repos do not have CCs or have so few that the CC may not be able to handle all the OSPRs. In this case, you would need to get 2U (or possibly tCRIL) reviewers
Backstage: https://backstage.openedx.org/catalog?filters%5Bkind%5D=component&filters%5Buser%5D=all
This tool allows you to see details about various GitHub repositories, including who the maintainers are. You can use this information to ping maintainers on PRs.
NOTE! This is still a pilot and only a handful of repos are represented.
openedx-webhooks Issues
https://github.com/openedx/openedx-webhooks/issues
This is where tickets related to the OSPR bot are tracked.
If you have suggestions for improvements make new tickets here and add those tickets to the https://github.com/orgs/openedx/projects/33/views/2 project board from the right side-bar on the issue.
Open TODO items
Update the bot to provide a welcome message saying test won’t run if you are a first time contributor
Come up with a list of common issues when running checks - OSPR Project Managers can link to this doc when people have questions
Sarina & Feanil will help create
First part done - message about how to get CLA help is now messaged when the bot makes a comment on a PR without a CLA; see pull request for bot changes.
Draft document up for the “doc for questions”. Not sure where to put it in the docs site, but comments can go here: https://docs.google.com/document/d/129cyHROlSXJRpt8O8TuqQTr6uVi-C7aY_6NgopFnq5k/edit?usp=sharing until I’m able to get a PR up in the proper place.
Finished TODO items
Make a GitHub group such as @openedx/cla-problems in GitHub, whose members are all the OSPR Project Managers, so people have an easy way to ask questions
Feanil will help create