Yep, I was describing an ideal scenario, and to achieve it we’d need to get around the problem that you mentioned.
Agreed, this could be a way to bootstrap increased CC coverage for individual repos.
That’s what I was trying to express, yes
–
If I were to summarize the current situation I’d say that both the CC program and the maintainership pilot are great initiatives. There just seems to be a bit of a disconnect between the two. We could probably gain a lot by implementing a process that would bring the two groups closer and enable them to support each other better, as well as by increasing CC coverage.
Gathering some data about where the longest delays are currently happening will probably yield additional and/or more specific insights.
There have been various discussions in different places related to this topic since the last post, so I’m giving a quick update here, for those who might not have seen them.
Maintainer program targeting edx-platform fully maintained
The maintainer pilot program is now entering phase 3, and a good news is that getting edx-platform fully maintained is being considered as one of its goals, as that it has been identified as one of the main blockers for PR reviews currently. Thanks for the input on this @Michelle_Philbrick & @itsjeyd !
On the front of getting core contributors involved as a backup when maintainers aren’t available, one of the blocker discussed with @Kelly_Buchanan is the fact that currently, trusting someone to merge on the project, equals to giving deployment rights on edx.org – so it’s definitely much harder to trust someone with merge rights, given the implications.
The usual solution discussed for this is forking, but many are wary of the impact on the project contributions and the amount of drift that would end up remaining on the fork – understandably so imho.
It’s still an important blocker to resolve though, as it will hinder collaboration and trust within the project until we can solve it. There is another other option though would not require to fork, would be to keep the automated deployments of the project master as they currently are, but introduce an approval/review step of the deployments on edx.org, when the code to be deployed has been merged without anyone from 2U being involved:
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
It could be automatic for most PRs only to add an approval of code deployments by someone from 2U, . Ie block the deployments until someone checked that the code merged is safe to deploy on edx.org (or has worked fixes/reverts to allow the build queue to resume). This way you’re sure that no code that hasn’t been reviewed by 2U ever reaches edx.org, without any of the risk of an actual fork, and with a strong incentive to quickly double-check the community’s independent review work. At the same time, merges by other community members become less risky, because they don’t immediately go on production without anyone from 2U looking.
TBH, I don’t see how this reduces risk or increases capacity. It delays the review until post-merge when the deployment pipeline is now blocked. Will the review then be somehow quicker than a review pre-merge? I don’t see how it would. But now it would be under the pressure of getting something else out the door. The overall capacity to review changes going to production hasn’t changed. The reviews have simply been delayed. Maybe I’m missing some subtlety here.
This also requires that the deployment tooling be changed to recognize the origin of the changes in the pipeline to be deployed.
If we can do quick reviews of community review work, let’s do it pre-merge, and keep things simple.
It doesn’t delay the review until post-merge – to be clear, the review would be the one done by the core contributor who would the merge, and would replace the maintainer’s review when maintainers aren’t available. The merge is what decides if a specific piece of code is accepted in the code base and the project. What would change is the edx.org-specific part, where you would get a second chance to do a review before it gets deployed to edx.org, and monitor the deployment itself – to avoid getting code pushed to production without anyone from 2U being aware or there to handle issues/revert if needed.
Of course, it’s better if a maintainer reviews before the merge itself - and there would still be an opportunity to do that pre-merge. What you would gain is a second opportunity before deployment, even if the code review is passed on to someone who isn’t from 2U.
The 2U review might not be quicker than a review pre-merge, but it would allow the rest of the community to be unblocked in the meantime. When we have to wait for months for a review, it means having to keep rebasing the work against the other evolutions of the codebase, and it prevents other community members from benefiting from the contributed work. The time & energy cost of a delayed review is borne entirely by the contributor.
Having to deal with the backlog of unreviewed upstream changes when wanting to push new changes isn’t the ideal situation, but it would balance the burden in these cases, and realign everyone’s incentives around getting all reviews done within reasonable delays.
Note that review is short for engineering review here.
For the repos that I’m responsible for these new fields have already been populated. (@Michelle_Philbrick I’m out of time for spending more time on this this week, but I could help update the fields for your repos starting next week.)
So you can now filter OSPRs based on how long they’ve been open/in the queue for engineering review. Examples:
For the record, I also renamed the Notes field to Last PM check to make the contents of that field easier to understand for people less familiar with the Contributions board.
It didn’t take a lot of time to populate the new fields retroactively. So if we wanted to be able to filter OSPRs by how long it took for them to get merged/closed, I think we could add another field for recording the merge date and populate that as well.
It has been a long thread with many different proposals to improve the project’s management of pull request reviews, so I thought it would be useful to list them summarized:
@antoviaque Thanks for summarizing all these points and creating GitHub issues for them
I can give it a shot, yep. Just noting that it might take a few sprints to complete, as I’ll have to timebox the work to leave enough time for OSPR management itself.
the merge, and would replace the maintainer’s review when maintainers aren’t available. The merge is what decides if a specific piece of code is accepted in the code base and the project. What would change is the edx.org-specific part, where you would get a second chance to do a review before it gets deployed to 