Re: [libvirt] [RFC] Accepting PRs/MRs for libvirt on GitHub/GitLab
On 10/16/19 9:22 AM, Andrea Bolognani wrote:
As we look to make the libvirt project easier to contribute to, one
fact that certainly comes to mind is that we only accept patches via
the mailing list. While the core developers are comfortable with the
email-based workflow and swear by it, many perspective contributors
are used to the PR/MR workflow common to many Open Source projects
these days, and similarly swear by it.
If we look at the PRs submitted on GitHub against the libvirt mirror
repository[1], there's just three of them per year on average, which
is arguably not a lot; however, it should be noted that each
repository also carries a fairly loud "PULL REQUESTS ARE IGNORED"
message right at the top, and it's not unlikely that a number of
perspective contributors simply lost interest after seeing it.
As a way to make contributions easier without forcing core developers
to give up their current workflow or making the project dependent on
a third-party provider, I suggest we adopt a hybrid approach.
First of all, we'd remove the ominous message from GitHub mirror
repositories (interestingly, the same is not present on GitLab).
Then, we'd starting accepting PRs/MRs. The way we'd handle them is
that, when one comes in, one among the handful of core developers who
volunteered to do so would review the patches on the respective
platform, working with the submitter to get it into shape just like
they would do on the mailing list; once the series is finally ready
to be merged, the core developer would update the PR/MR as necessary,
for example picking up R-bs or fixing the kind of trivial issues that
usually don't warrant a repost, and then push to master as usual.
This would mean that there will be patches that will get accepted
without the ML being aware of. The core developer (or the author
itself) would need to send the revised patches to the ML before
pushing to make people aware of it before pushing to master, IMO.
I have a 2 year experience maintaining a now defunct project in
Github (a KVM web management tool called Kimchi) in which we used a
hybrid approach like I think you're suggesting, with mailing list, people
opening bugs in Github + Github PRs. It was annoying - people will
inevitably
discuss issues using the Github tracking system, which means that you'll
have to subscribe via email to Github notifications if you didn't want
to miss
out. It was common for people that used the ML to start discussions that
were already happening in the Web, and vice-versa.
All that to make a case against Libvirt having additional ways of
communication. I don't mind pull requests from Github/Gitlab as long as the
ML is made aware of, before the patches are accepted. But people bringing
up discussions in the ML and Github/Gitlab scales poorly, in my experience.
DHB
More in detail: GitHub and GitLab have a feature that allows project
members to update PRs/MRs: basically the way it works is that, if the
PR/MR was created by the user 'foo' from their branch 'bar', the
libvirt developer is allowed to (force-)push to the foo/libvirt/bar
branch, and doing so updates the corresponding PR/MR; after that, if
the updated branch is locally merged into master and master is pushed
to the libvirt.org repo, once it gets mirrored GitHub/GitLab will
recognize that the commit IDs match and automatically mark the PR/MR
as merged. I have tested this behavior on both platforms (minus the
mirroring part) with Martin's help.
One last important bit. In the spirit of not requiring core
developers to alter their workflow, the developer who reviewed the
patches on GitHub/GitLab will also be responsible to format them to
the mailing list after merging them: this way, even those who don't
have a GitHub/GitLab account will get a chance to take notice of the
code changes and weigh in. Unlike what we're used to, this feedback
will come after the fact, but assuming that issues are spotted only
at that point we can either push the relevant fixes as follow-up
commits or even revert the series outright, so I don't feel like it
would be a massive problem overall.
Thoughts?
[1] https://github.com/libvirt/libvirt/pulls?q=is:pr+is:closed