Re: cfbot update: Using GitHub for patch review
On Sat, Jun 29, 2024 at 2:12 PM Jelte Fennema-Nio wrote: > On Sat, 29 Jun 2024 at 01:13, Thomas Munro wrote: > > > > On Sat, Jun 29, 2024 at 1:10 AM Ashutosh Bapat > > 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 like to see that one day, getting a consensus and changing the process for whole community is a lot of effort. I didn't think (or mean) that we would move our review process to Github with this change. Sorry if it sounded like that. > > 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). > It is not a small amount of work, I agree. But it may be a way forward. Those who want to use PR for review can review them as long as the reviews are visible on the mailing list. Many of us already draft our review emails similar to how it would look like in a PR. If the PR system can send that email on reviewer's behalf (as if it's sent by the reviewer) it will integrate well with the current process. People will learn, get used to it and move eventually to PR based reviews. > > 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. > +1. -- Best Wishes, Ashutosh Bapat
Re: cfbot update: Using GitHub for patch review
On Sat, 29 Jun 2024 at 01:13, Thomas Munro wrote: > > On Sat, Jun 29, 2024 at 1:10 AM Ashutosh Bapat > 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
Re: cfbot update: Using GitHub for patch review
On Sat, Jun 29, 2024 at 1:10 AM Ashutosh Bapat 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 decide to 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.
Re: cfbot update: Using GitHub for patch review
On Fri, Jun 21, 2024 at 8:06 PM 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. > > 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. > Thanks. Very helpful. Will it be possible to make it send an email containing the review comments? Better even if a reply to that email adds comments/responses back to PR. 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. There was some discussion at pgconf.dev about using gitlab instead of github. How easy is it to use gitlab if we decide to go that way? -- Best Wishes, Ashutosh Bapat
Re: cfbot update: Using GitHub for patch review
pá 21. 6. 2024 v 17:55 odesílatel Nathan Bossart napsal: > 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. +1 good work Pavel > > > -- > nathan > > >
Re: cfbot update: Using GitHub for patch review
pá 21. 6. 2024 v 16:36 odesílatel Jelte Fennema-Nio 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 > >
Re: cfbot update: Using GitHub for patch review
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
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