Hello everyone, I'm working on GitHub integration as a part of GSoC. One of my tasks was to show GitHub review comments on b.p.o. Here is how I did it :- I wrote a handler to handle GitHub PR review/diff comments event using webhooks. If a b.p.o issue is linked to a PR, then it checks if any "GitHub" comment was added in last "X" minutes. If no GitHub comments were added, then it adds that comment to the linked issue. This way there will be fewer emails and also developers who are in the nosy list but are not subscribed to GitHub PR will get notified about the progress/review comments on PR. You can check the patch here <http://psf.upfronthosting.co.za/roundup/meta/issue592>. I wanted to know from you all if this is the right approach. If not, then what should be changed? Thank you, Anish Shah
On Wed, Jul 27, 2016 at 1:00 PM, Anish Shah <shah.anish07@gmail.com> wrote:
it checks if any "GitHub" comment was added in last "X" minutes. If no GitHub comments were added, then it adds that comment to the linked issue.
I guess, you meant, "if any comments were added to GitHub issues". So, it does polling instead of any real-time event notifications. The Github PR comments also have a context associated with it, which are are useful while reviewing. Adding those context in the b.p.o comments can cause a churn. Instead of adding comments to b.p.o, have you considered sending only the emails notifications nosy list? Just like reitveld does right now? -- Senthil
On Wed, 27 Jul 2016 14:00:09 -0700, Senthil Kumaran <senthil@uthcode.com> wrote:
On Wed, Jul 27, 2016 at 1:00 PM, Anish Shah <shah.anish07@gmail.com> wrote:
it checks if any "GitHub" comment was added in last "X" minutes. If no GitHub comments were added, then it adds that comment to the linked issue.
I guess, you meant, "if any comments were added to GitHub issues". So, it does polling instead of any real-time event notifications.
The Github PR comments also have a context associated with it, which are are useful while reviewing. Adding those context in the b.p.o comments can cause a churn.
I've never found that context useful, myself. It should be possible to mechanically strip it.
Instead of adding comments to b.p.o, have you considered sending only the emails notifications nosy list? Just like reitveld does right now?
The general consensus (a while back) was that the fact that reitveld doesn't update the bug tracker issue is a bug, and we're trying to fix that :) --David
On Wed, Jul 27, 2016 at 2:57 PM, R. David Murray <rdmurray@bitdance.com> wrote:
The general consensus (a while back) was that the fact that reitveld doesn't update the bug tracker issue is a bug, and we're trying to fix that :)
Haha. Interesting. Github sends one email for every comment, it does not have a "draft" + "publish" comments mechanism that Rietveld has. This has the potential to create a lot of churn for some patches/reviews.
On Wed, 27 Jul 2016 16:08:52 -0700, Senthil Kumaran <senthil@uthcode.com> wrote:
On Wed, Jul 27, 2016 at 2:57 PM, R. David Murray <rdmurray@bitdance.com> wrote:
The general consensus (a while back) was that the fact that reitveld doesn't update the bug tracker issue is a bug, and we're trying to fix that :)
Haha. Interesting.
Github sends one email for every comment, it does not have a "draft" + "publish" comments mechanism that Rietveld has. This has the potential to create a lot of churn for some patches/reviews.
Yes, that's the idea behind Amash's 30 minute window (or whatever it is or we decide to make it): to accumulate multiple github review comments into one post to bugs. --David
On Wed, 27 Jul 2016 16:08:52 -0700, Senthil Kumaran <senthil@uthcode.com> wrote:
On Wed, Jul 27, 2016 at 2:57 PM, R. David Murray <rdmurray@bitdance.com> wrote:
The general consensus (a while back) was that the fact that reitveld doesn't update the bug tracker issue is a bug, and we're trying to fix that :)
Haha. Interesting.
Github sends one email for every comment, it does not have a "draft" + "publish" comments mechanism that Rietveld has. This has the potential to create a lot of churn for some patches/reviews.
The obvious alternative is to just post a link to the PR with some summary about what was posted (perhaps who and how many comments?). I don't have an opinion at this point which would be better, but the core of the proposed patch would be the same, just the output would be different. --David
On Thu, Jul 28, 2016 at 7:48 AM, R. David Murray <rdmurray@bitdance.com> wrote:
The obvious alternative is to just post a link to the PR with some summary about what was posted (perhaps who and how many comments?). I don't have an opinion at this point which would be better, but the core of the proposed patch would be the same, just the output would be different.
This sounds better to me. If the person in the nosy list is interested, they can be added to the nosy list of PR, so that they can subsequent updates from there. Thank you, Senthil
On 07/28/2016 06:21 PM, Senthil Kumaran wrote:
On Thu, Jul 28, 2016 at 7:48 AM, R. David Murray <rdmurray@bitdance.com> wrote:
The obvious alternative is to just post a link to the PR with some summary about what was posted (perhaps who and how many comments?). I don't have an opinion at this point which would be better, but the core of the proposed patch would be the same, just the output would be different.
This sounds better to me. If the person in the nosy list is interested, they can be added to the nosy list of PR, so that they can subsequent updates from there.
One case for copying comments is to create an archive for the case that Github goes bankrupt or turns evil. This seems very unlikely now, but Python should be planning for long-term contingencies. AFAIK there's no formal agreement between the PSF and Github that PR archives will remain accessible.
There has been talk in the past of setting up a cronjob somewhere that backs up all data stored at GitHub in some way for contingency purposes. I think it's a totally reasonable thing to do, but I don't plan to hold up the migration for it. On Fri, 29 Jul 2016 at 04:09 Petr Viktorin <encukou@gmail.com> wrote:
On 07/28/2016 06:21 PM, Senthil Kumaran wrote:
On Thu, Jul 28, 2016 at 7:48 AM, R. David Murray <rdmurray@bitdance.com> wrote:
The obvious alternative is to just post a link to the PR with some summary about what was posted (perhaps who and how many comments?). I don't have an opinion at this point which would be better, but the core of the proposed patch would be the same, just the output would be different.
This sounds better to me. If the person in the nosy list is interested, they can be added to the nosy list of PR, so that they can subsequent updates from there.
One case for copying comments is to create an archive for the case that Github goes bankrupt or turns evil. This seems very unlikely now, but Python should be planning for long-term contingencies. AFAIK there's no formal agreement between the PSF and Github that PR archives will remain accessible.
_______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct
I'm leaning towards just adding an information who left the comment and a link to the PR. I agree with Senthil that gh comments, especially those coming from reviews have code context, which will get lost when copying over. Besides the amount does not matter in that case, whoever is interested in looking or answering into the patch will have to go to GitHub and see what exactly it's about. I'm aware there are cases you just want to read the comment and don't do anything yet, but these are rare cases we can initially ignore. Let's start simple and we can always get back to this topic. On Fri, Jul 29, 2016 at 7:04 PM, Brett Cannon <brett@python.org> wrote:
There has been talk in the past of setting up a cronjob somewhere that backs up all data stored at GitHub in some way for contingency purposes. I think it's a totally reasonable thing to do, but I don't plan to hold up the migration for it.
On Fri, 29 Jul 2016 at 04:09 Petr Viktorin <encukou@gmail.com> wrote:
On 07/28/2016 06:21 PM, Senthil Kumaran wrote:
On Thu, Jul 28, 2016 at 7:48 AM, R. David Murray <rdmurray@bitdance.com> wrote:
The obvious alternative is to just post a link to the PR with some summary about what was posted (perhaps who and how many comments?). I don't have an opinion at this point which would be better, but the core of the proposed patch would be the same, just the output would be different.
This sounds better to me. If the person in the nosy list is interested, they can be added to the nosy list of PR, so that they can subsequent updates from there.
One case for copying comments is to create an archive for the case that Github goes bankrupt or turns evil. This seems very unlikely now, but Python should be planning for long-term contingencies. AFAIK there's no formal agreement between the PSF and Github that PR archives will remain accessible.
_______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct
_______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct
On Sat, 30 Jul 2016 23:21:07 +0200, Maciej Szulik <soltysh@gmail.com> wrote:
I'm leaning towards just adding an information who left the comment and a link to the PR. I agree with Senthil that gh comments, especially those coming from reviews have code context, which will get lost when copying over. Besides the amount does not matter in that case, whoever is interested in looking or answering into the patch will have to go to GitHub and see what exactly it's about. I'm aware there are cases you just want to read the comment and don't do anything yet, but these are rare cases we can initially ignore. Let's start simple and we can always get back to this topic.
If github comment threading were more sensible I think I'd prefer to see the comments reflected. But since it *isn't* (it is pretty much useless outside of the web UI, and even in the web UI it is often awkward), I think linking to the PR is indeed probably better. Just to confirm, we are talking about a new link summarizing the comment activity for the past N minutes, whenever commenting activity happens, right? It would be nice to link directly to the new comments, but somehow I doubt that is going to be possible (at least if we batch them), so we'll probably have to settle for just linking the summary to the PR as a whole. --David
On Tue, Aug 2, 2016 at 4:24 PM, R. David Murray <rdmurray@bitdance.com> wrote:
On Sat, 30 Jul 2016 23:21:07 +0200, Maciej Szulik <soltysh@gmail.com> wrote:
I'm leaning towards just adding an information who left the comment and a link to the PR. I agree with Senthil that gh comments, especially those coming from reviews have code context, which will get lost when copying over. Besides the amount does not matter in that case, whoever is interested in looking or answering into the patch will have to go to GitHub and see what exactly it's about. I'm aware there are cases you just want to read the comment and don't do anything yet, but these are rare cases we can initially ignore. Let's start simple and we can always get back to this topic.
If github comment threading were more sensible I think I'd prefer to see the comments reflected. But since it *isn't* (it is pretty much useless outside of the web UI, and even in the web UI it is often awkward), I think linking to the PR is indeed probably better.
Just to confirm, we are talking about a new link summarizing the comment activity for the past N minutes, whenever commenting activity happens, right? It would be nice to link directly to the new comments, but somehow I doubt that is going to be possible (at least if we batch them), so we'll probably have to settle for just linking the summary to the PR as a whole.
Correct. The links to separate comments might be of no use after a rebase to a PR, since they will point to hidden comments, that's why having a single global link is much better, because going to the PR will give you the current state of it. Maciej
On Thu, 11 Aug 2016 22:09:33 +0200, Maciej Szulik <soltysh@gmail.com> wrote:
On Tue, Aug 2, 2016 at 4:24 PM, R. David Murray <rdmurray@bitdance.com> wrote:
On Sat, 30 Jul 2016 23:21:07 +0200, Maciej Szulik <soltysh@gmail.com> wrote:
I'm leaning towards just adding an information who left the comment and a link to the PR. I agree with Senthil that gh comments, especially those coming from reviews have code context, which will get lost when copying over. Besides the amount does not matter in that case, whoever is interested in looking or answering into the patch will have to go to GitHub and see what exactly it's about. I'm aware there are cases you just want to read the comment and don't do anything yet, but these are rare cases we can initially ignore. Let's start simple and we can always get back to this topic.
If github comment threading were more sensible I think I'd prefer to see the comments reflected. But since it *isn't* (it is pretty much useless outside of the web UI, and even in the web UI it is often awkward), I think linking to the PR is indeed probably better.
Just to confirm, we are talking about a new link summarizing the comment activity for the past N minutes, whenever commenting activity happens, right? It would be nice to link directly to the new comments, but somehow I doubt that is going to be possible (at least if we batch them), so we'll probably have to settle for just linking the summary to the PR as a whole.
Correct. The links to separate comments might be of no use after a rebase to a PR, since they will point to hidden comments, that's why having a single global link is much better, because going to the PR will give you the current state of it.
Yeah, that's one of the things that makes even using it through the web UI problematic. Oh well :) --David
participants (6)
-
Anish Shah -
Brett Cannon -
Maciej Szulik -
Petr Viktorin -
R. David Murray -
Senthil Kumaran