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 Axim 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 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.

Specific Responsibilities

What follows is a set of questions and answers to situations 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 Axim 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 is why it’s important to get things merged sooner rather than later (as the master branch tends to change).

  • Conflict = Someone changed information in the same file 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 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).

“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 that 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 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.

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 (e.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 it to stay open unless 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-contribution label on it:
  

  

  PRs generated by 2U and Axim 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 previously committed to a different repo) their PR needs to have approval before checks are run (since the tests execute code).

  • Repository maintainers and Axim engineers are the ones who should press the button allowing tests to run, after they’ve examined the code for potential maliciousness.

  • 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 Axim engineers know whether a PR is safe to run tests on (or not).

• Failed checks are authors' responsibility except for needing to get a 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 it 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.

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 Axim ticket to register them in the system.

  • Their team must know what this means; it 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 and Resources

External Contributions GitHub Board

https://github.com/orgs/openedx/projects/19/views/1  

• PRs not authored by 2U or Axim engineers go to this board when they come in.

• Once added to the board, they need to get reviewed and merged.

• Use the board’s columns to change the status of a PR (you can also change it on the right side of the PR page itself). The following guidelines should help determine appropriate status (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 review and formal approval for a PR and merge it.

• Many repos do not have CCs or have so few that they may not be able to handle all incoming OSPRs. In this case, you would need to get 2U (or possibly Axim) reviewers to approve and merge PRs.

Contributions Board Status and Labels for Open Source Contributions

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.

Issue tracker: openedx-webhooks

https://github.com/openedx/openedx-webhooks/issues

• This is where issues related to the OSPR bot are tracked.

• If you have suggestions for improvements, create new issues here and add them to the https://github.com/orgs/openedx/projects/33/views/2 project board from the right sidebar on the issue.

Current OSPR Project Managers

• Michelle Philbrick (Axim, @mphilbrick211 on GitHub)

• Tim Krones (OpenCraft, @itsjeyd on GitHub)

Open TODO items

1. Update the bot to provide a welcome message saying test won’t run if you are a first time contributor

2. Come up with a list of common issues when running checks - OSPR Project Managers can link to this doc when people have questions

   1. Sarina & Feanil will help create

   2. 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.

   3. 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

1. 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

   1. Feanil will help create