Thread: cfbot update: Using GitHub for patch review

I recently got write access to the cfbot repo[1] and machine from
Thomas. And I deployed a few improvements this week. The most
significant one is that it is now much easier to use GitHub as part of
your patch review workflow.

On the cfbot website[2] there's now a "D" (diff) link next to each
commit fest entry.  A good example of such a link would be the one for
my most recent commitfest entry[3]. There is a separate commit for
each patch file and those commits contain the "git format-patch"
metadata. (this is not done using git am, but using git mailinfo +
patch + sed, because git am is horrible at resolving conflicts)

The killer feature (imho) of GitHub diffs over looking at patch files:
You can press the "Expand up"/"Expand down" buttons on the left of the
diff to see some extra context that the patch file doesn't contain.

You can also add the cfbot repo as a remote to your local git
repository. That way you don't have to manually download patches and
apply them to your local checkout anymore:

# Add the remote
git remote add -f cfbot https://github.com/postgresql-cfbot/postgresql.git
# make future git pulls much quicker (optional)
git maintenance start
# check out a commitfest entry
git checkout cf/5065

P.S. Suggestions for further improvements are definitely appreciated.
We're currently already working on better integration between the
commitfest app website and the cfbot website.

P.P.S The "D" links don't work for patches that need to be rebased
since before I deployed this change, but that problem should fix
itself with time.

[1]: https://github.com/macdice/cfbot
[2]: http://cfbot.cputube.org/
[3]: https://github.com/postgresql-cfbot/postgresql/compare/cf/5065~1...cf/5065

On Fri, Jun 21, 2024 at 04:36:13PM +0200, Jelte Fennema-Nio wrote:
> I recently got write access to the cfbot repo[1] and machine from
> Thomas. And I deployed a few improvements this week. The most
> significant one is that it is now much easier to use GitHub as part of
> your patch review workflow.

Nice!  Thank you.

-- 
nathan

pá 21. 6. 2024 v 16:36 odesílatel Jelte Fennema-Nio <postgres@jeltef.nl> napsal:
>
> I recently got write access to the cfbot repo[1] and machine from
> Thomas. And I deployed a few improvements this week. The most
> significant one is that it is now much easier to use GitHub as part of
> your patch review workflow.
>
> On the cfbot website[2] there's now a "D" (diff) link next to each
> commit fest entry. A good example of such a link would be the one for
> my most recent commitfest entry[3]. There is a separate commit for
> each patch file and those commits contain the "git format-patch"
> metadata. (this is not done using git am, but using git mailinfo +
> patch + sed, because git am is horrible at resolving conflicts)

This is brilliant!

> The killer feature (imho) of GitHub diffs over looking at patch files:
> You can press the "Expand up"/"Expand down" buttons on the left of the
> diff to see some extra context that the patch file doesn't contain.
>
> You can also add the cfbot repo as a remote to your local git
> repository. That way you don't have to manually download patches and
> apply them to your local checkout anymore:
>
> # Add the remote
> git remote add -f cfbot https://github.com/postgresql-cfbot/postgresql.git
> # make future git pulls much quicker (optional)
> git maintenance start
> # check out a commitfest entry
> git checkout cf/5065
>
> P.S. Suggestions for further improvements are definitely appreciated.
> We're currently already working on better integration between the
> commitfest app website and the cfbot website.
>
> P.P.S The "D" links don't work for patches that need to be rebased
> since before I deployed this change, but that problem should fix
> itself with time.
>
> [1]: https://github.com/macdice/cfbot
> [2]: http://cfbot.cputube.org/
> [3]: https://github.com/postgresql-cfbot/postgresql/compare/cf/5065~1...cf/5065
>
>

On Sat, Jun 29, 2024 at 1:10 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> I need to sign in to github to add my review comments. So those who do not have a github account can not use it for
review.But I don't think that can be fixed. We need a way to know who left review comments. 

I don't think Jelte was talking about moving review discussion to
Github, just providing a link to *view* the patches there.  Now I'm
wondering if there is a way to disable comments on commits in the
postgresql-cfbot GH account.  I guess they'd be lost after 48 hours
anyway when the branch gets force-pushed and commit hash changes?  I
don't want people to start posting comments there that no one is
looking at.

> There was some discussion at pgconf.dev about using gitlab instead of github. How easy is it to use gitlab if we
decideto go that way? 

cfbot could certainly be configured to push (ie mirror) the same
branches to gitlab too (I don't have much experience with Gitlab, but
if it's just a matter of registering an account + ssh key, adding it
as a remote and pushing...).  Then there could be [View on Github]
[View on Gitlab] buttons, if people think that's useful (note "View",
not "Review"!).  The Cirrus CI system is currently only capable of
testing stuff pushed to Github, though, so cfbot would continue to
push stuff there.

If memory servers, Cirrus used to say that they were planning to add
support for testing code in public Gitlab next, but now their FAQ says
their next public git host will be Bit Bucket:
https://cirrus-ci.org/faq/#only-github-support

Given that cfbot is currently only using Github because we have to to
reach Cirrus CI, not because we actually want Github features like
issue tracking or pull requests with review discussion, it hardly
matters if it's Github, Gitlab or any other public git host.  And if
we eventually decide to move our whole workflow to one of those
systems and shut down the CF app, then cfbot will be retired, and
you'll just create PRs on that system.  But so far, we continue to
prefer the CF app + email.

The reason we liked Cirrus so much despite the existence of many other
CI systems including the ones build into GH, GL, etc and many 3rd
party ones, was because it was the only provider that allowed enough
compute minutes for our needs, supported lots of operating systems,
and had public links to log files suitable for sharing on out mailing
list or cfbot's web interface (if you click to see the log, it doesn't
say "Rol up roll up, welcome to Foo Corporation, get your tickets
here!").  I still don't know of any other CI system that would be as
good for us, other than building our own.  I would say it's been a
very good choice so far.  The original cfbot goal was "feed the
mailing list to a CI system", with Github just a necessary part of the
plumbing.  It is a nice way to view patches though.

On Sat, 29 Jun 2024 at 01:13, Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Sat, Jun 29, 2024 at 1:10 AM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> > I need to sign in to github to add my review comments. So those who do not have a github account can not use it for
review.But I don't think that can be fixed. We need a way to know who left review comments. 
>
> I don't think Jelte was talking about moving review discussion to
> Github, just providing a link to *view* the patches there.

Totally correct. And I realize now I should have called that out
explicitly in the initial email.

While I personally would love to be able to read & write comments on a
Github PR, integrating that with the mailing list in a way that the
community is happy with as a whole is no small task (both technically
and politically).

So (for now) I took the easy way out and sidestepped all those
difficulties, by making the github branches of the cfbot (which we
already had) a bit more user friendly as a way to access patches in a
read-only way.

>  Now I'm
> wondering if there is a way to disable comments on commits in the
> postgresql-cfbot GH account.  I guess they'd be lost after 48 hours
> anyway when the branch gets force-pushed and commit hash changes?  I
> don't want people to start posting comments there that no one is
> looking at.

It seems you can disable them for 6 months at a time here:
https://github.com/postgresql-cfbot/postgresql/settings/interaction_limits