Core contributors as backup reviewers

[antoviaque]

This issue is to follow up on a topic from the contributor meetup working group

See https://discuss.openedx.org/t/pull-requests-review-delays/10497 for the context

• Enlist core contributors to review OSPRs:

  • From unmaintained repositories, as well as repositories on which the maintainer is unavailable.
  • This would allow a binding 👍 from the core contributor, which would be able to merge the code (through a bot if necessary)

• Determine how to label OSPRs to differentiate PRs that are:

  • Good for CC-only review (the default, when there is no maintainer, or answer after X days?)
  • Good for CC review, but a specific maintainer will need to give final 👍, or
  • Not a good candidate for CC review; a specific maintainer will absolutely need to review it

• Do experiments: find out specific PRs that could be passed on to a CC, and find volunteers; check issue about tracking of OSPRs

  • Build experience and trust. And the more opportunities we have for interactions and collaborations like that, the more trust can develop.
  • Ned & her already escalation step
    • -> When escalating specific PRs, ask if there are objections to escalate to the community?
    • Mention to Kelly the filters to see stuck PRs

• Make it safer to merge community code on master: add manual reviews of deployments on edx.org:

  • If a PR merges on master that is from someone from 2U or has been approved by someone from 2U => automated deployment, like now
  • If a PR merges on master without anyone from 2U involved => automated deployment stopped, until someone double-checks the merge, and greenlights the deployment

• Maybe every few weeks Tim and Michelle could post stalled ones in the CC slack channel / discourse and have people self-assign?
  • Michelle: currently I run the OSPRs that need review on edx-platform by Ed, and he helps to guide me to someone to review/merge. Would it be helpful to post these in the Maintainer channel instead so you can weigh in too? I typically do them in batches every couple of weeks.
• => maybe a bot could help? Eg. X days after the PR goes out of draft or when the repo is unmaintained, a bot posts an automated message on the PR comments, saying that it can now be reviewed and approved or refused by any code core contributor.
  • bot automatically ping the right people for the review (needs a list of core contributors for the code)
  • The bot could do the merge / close of the PR based on

[antoviaque]

This has been implemented, core contributors are now backup reviewers on PRs without maintainers, or when the maintainers are unresponsive. See:

• https://discuss.openedx.org/t/pull-requests-review-delays/10497/34
• https://openedx.atlassian.net/wiki/spaces/COMM/pages/4031578215/2024-02-01+Meeting+notes#%E2%A4%B4-Decisions

The last step on this is for @itsjeyd to announce it more broadly to make sure everyone is aware.

[antoviaque]

And if you are a core contributor, go pick a PR to review at the link Tim provided:

If you are a CC for one or more repositories, you are welcome to check the Contributions board for OSPRs that are ready for review – and/or have been stuck waiting for review for more than two weeks) – and start reviewing them!

[antoviaque]

@mphilbrick211 @itsjeyd Given that now this has been an option more widely available for PR reviews, how is that working out when you try to assign reviews? Looking at the list of PRs up for review, it seem that only a few of them could be assigned? Is it an issue with identifying potential core contributor reviewers, or negative/no answers from the core contributors?

[mphilbrick211]

Hi @antoviaque - I am going through my OSPRs and aligning them with the new maintainer info. I'm then grouping them into OPRPs that are stalled / need review and will be reaching out to CCs where needed. I have hundreds of pull requests so it's taken a bit to get them organized, but I'll be reaching out to CCs soon.

[mphilbrick211]

@antoviaque I should add that I don't usually use the "assignee" field, and instead I use the "reviewer" field.

Also keep in mind that some of those on the list are from repos like "ecommerce" which are no longer in use so they may eventually be closed. Others have reviewers that are no longer reviewing (e.g. they may have been assigned for a 2U team to review, but that team may no longer be reviewing pull requests) - and in these cases, I'm trying to organize them as well.

[antoviaque]

@mphilbrick211 Thanks for the details! That's a good point about the assignee vs reviewers fields, I was looking at the wrong field :) Displaying the reviewers on the table paints a much better picture, that's great.

We discussed that point during the contributors meetup yesterday, and @arbrandes pointed out that the difference between assignee and reviewers on github can be confusing - the reviewers fields is actually quite straightforward (it lists the people who should be reviewing the PR), but there is no definition of what the assignee field is for, in a PR. On an issue it is the person fixing it, but for a PR the author is already the one responsible for getting the PR to the finish line - which might be why it's often not set?

Btw, @arbrandes also mentioned that he now reviews any PR where he has merge rights as a core contributor, without waiting on others or maintainers, to avoid the long delays we often have because of the long/low response times of some people/teams - which makes sense to me, this is the way it usually works best: the first person who is able to review does it.

[itsjeyd]

@antoviaque

@arbrandes ... now reviews any PR where he has merge rights as a core contributor, without waiting on others or maintainers, to avoid the long delays we often have because of the long/low response times of some people/teams

That sounds great 🙂

... there is no definition of what the assignee field is for, in a PR.

This point has come up in a few different conversations about OSPR management over time, and there have been some attempts to start using the Assignee field to indicate responsibility for shepherding a PR through the review process (either through self-assignment by maintainers or through @mphilbrick211 and I setting the assignee after getting confirmation from someone in the PR comments that they were going to take care of the review).

So far this approach hasn't fully taken off, though, and is not being done consistently at the moment.

[antoviaque]

@itsjeyd That would make sense to me 👍

[antoviaque]

@itsjeyd @mphilbrick211 This has now been in place for a bit, and it seem to be going well? Any issues or blockers remaining to push PR reviews to core contributors?

[itsjeyd]

@antoviaque Yep, it's definitely been helpful to be able to bring core contributors into the review process without having to wait for explicit approval from maintainers.

Right now, the main point of friction that remains for me is that there's no quick way of getting a complete list of core contributors for a given repo. (And I'd imagine that for edx-platform, there's some added complexity in terms of knowing who is familiar with which area of the platform, and therefore best suited to review a given PR. Maybe @mphilbrick211 can confirm.)

We have a mapping of core contributors to repos on the wiki, but (as far as I'm aware) no mapping from repos to core contributors. And if someone is a CC for multiple repos, the wiki doesn't necessarily list all of them. (It provides a link to the GitHub teams that they are part of instead.) So doing a browser search on the wiki page for the name of a repo will give incomplete results, in that it will only match CCs that have the repo listed explicitly next to their name.

Maybe this could be addressed through some sort of automation involving catalog-info.yaml (which works well for making info about who maintains which repo discoverable via Backstage and @feanil's spreadsheet).

[antoviaque]

@itsjeyd Thanks for the details! It's good to know that getting the core contributors involved helps. :)

Maybe this could be addressed through some sort of automation involving catalog-info.yaml (which works well for making info about who maintains which repo discoverable via Backstage and @feanil's spreadsheet).

👍 Do you think we could schedule a discovery to investigate this approach? If you don't have time, don't hesitate to involve another core contributor - there time left in some of the core contributors' budgets.

[itsjeyd]

@antoviaque Yep, I'll post about it on the core contributors epic before the end of the sprint 👍

[antoviaque]

@itsjeyd I believe this has progressed right, with an upcoming change to the bot answering PRs to give instructions about who to ping and what the PR author can do to get their PR reviewed?

I'm not sure if we now have a clear mapping of who reviews what though using backstage / catalog-info.yaml?

[itsjeyd]

@antoviaque Yes, we have openedx/openedx-webhooks#288 which updates the welcome message for OSPRs, and enables the bot to provide info about who the maintainers of the parent repo of a given PR are. This info is based on catalog-info.yaml. (Check out the new welcome message here if you're interested.)

The issue that I mentioned above (= no mapping between repos and CCs) is still open, though. I had posted about it internally before the conference but there were no takers so far.

[antoviaque]

@itsjeyd Thanks for the recap - I have pinged people on this. Don't hesitate to escalate it when you get no answer on a ping, especially on an important task like this. I'll create a dedicated ticket for it on this board to follow up

[itsjeyd]

Will do, thanks @antoviaque 👍