...
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.
...
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 from PRs.
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.
...
What follows is a set of questions and answers to situations the that an OSPR Project Manager may face. There will certainly be situations that arise that are not answered here. Questions about these 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 their changes in this case; this . This is why it’s important to get things merged sooner rather than later (as the master branch tends to change).
Conflict = Someone changed information on in the same file ; the on the target branch of the PR (which is usually master). The author has to reconcile those changes with their own.
“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 OSPR Project 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).
...
Review notes can be found under the “files changed” tab; this . 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 that their requested changes are resolved before the PR can be merged.
...
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 include 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.
...
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 (ege.g., “Thank you for your contribution! We’re closing this PR 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 it to stay open unless they’re it’s been 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.
...
For contributors on a new repo (even if they’ve previously committed in to a different repo) their PR needs to have approval before checks are run (since the tests execute code).
If an OSPR Project Manager has a technical background (to the extent that they’re able to judge whether a PR is introducing malicious code), they may check code changes themselves and let repository maintainers and/or tCRIL engineers know whether a PR is safe to run tests on (or not).
Failed checks are authors' responsibility except for needing to complete get a CLA (pull in Feanil/Michelle)green CLA check.
If author has signed a CLA but the 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 not, pull in Feanil/Michelle.
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 to ping 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 PR as this doesn’t change codeit won’t be merged in this state (and therefore won’t affect production 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.
...
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 it is not your job to walk them through a process people in their org know!
...
https://github.com/orgs/openedx/projects/19/views/1
Tickets PRs not authored by 2U or tCRIL engineers go to this board when they come in.
Once added to the board, it needs they need to get reviewed and merged.
Use the board’s columns to change state the status of a PR (you can also change it on the right side of the PR pagespage itself). 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 assignstatus
.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
(as well as labels to add to the PR to capture additional information): Contributions Board Status & Label Breakdown.
Core Contributors and their repos:Core Contributors to the Open edX Project
CCs on a repo can provide an approval review and formal approval for a PR and merge a PRit.
Many repos do not have CCs or have so few that the CC they may not be able to handle all the incoming OSPRs. In this case, you would need to get 2U (or possibly tCRIL) reviewers to approve and merge PRs.
Backstage: https://backstage.openedx.org/catalog?filters%5Bkind%5D=component&filters%5Buser%5D=all
...
https://github.com/openedx/openedx-webhooks/issues
This is where tickets issues related to the OSPR bot are tracked.
If you have suggestions for improvements make , create new tickets issues here and add those tickets them to the https://github.com/orgs/openedx/projects/33/views/2 project board from the right side-bar sidebar on the issue.
Open TODO items
...