[Twisted-Python] Defining the review workflow on top of GitHub PR
I am restarting this discussion https://twistedmatrix.com/pipermail/twisted-python/2016-May/030333.html I am starting a new thread since I want to keep the focus on the review process / workflow / markers, and not on the things required to accept a PR or do a review. ----------
Proposing: Just open a pull request. Any open pull request should be treated as part of the queue.
I don't like this. If you are not a comitter, you need to open a PR to trigger the tests. So you want to first open a PR, then wait for tests to execute, then fix and only after that to request the review. We can start with setting the title to have "[WIP]" marker, to let others know that this is not yet ready... but then when changes are required, the reviewer will have to set the WIP marker again.. and if the reviewer is not a team member, it will not have rights to edit the subject. But I hope that we can have a bot which once a "please review" comment is left, it will set a label.
Accepting: A committer pushes the big green button;
+1 ... but maybe also leave a comment :)
Reviewing: This is the potentially slightly odd part. I believe a review that doesn't result in acceptance should close the PR. We need to be careful to always include some text that explains that closing a PR does not mean that the change is rejected, just that the submitter must re-submit. Initially this would just mean opening a new PR, but Mark Williams is working on a bot to re-open pull requests when a submitter posts a "please review" comment: https://github.com/markrwilliams/txghbot
Since we will have a bot for "please review", why not use the same bot to set a label on "please make changes" ? I think that closing a PR should mean that the work on that branch is rejected :)
Responding: A submitter can open a new PR, or, once we start running txghbot, reopen their closed PR.
As commented above, I am +1 for leaving a "please review" comment and having a bot updating the labels.
Viewing: https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-status%3Afailed
One we get the "please-review" and "changes-needed" labels it should be eaiser to view the queue. ------- Whem multiple reviewers are required, you can use the dedicated GitHub Review message and approve it without hiting the merge button. --------- I have no idea how other projects are managing the review queue. Please send your feedback. If we agree on a process based on managing the labes, I can work on implemeting the required logic with a bot and GitHub hooks. -------- We can also start by using the WIP marker * while preparing the PR * once changes are required and the author works on addressing the changes requsted on review Any PR which is open and does not have the WIP marker means that is part of the queue. ---------- Thanks! PS: I have checked pyca/crypography but I don't see any pattern there and a lot of PR are merged without any comment https://github.com/pyca/cryptography/pulls?q=is%3Apr+is%3Aclosed -- Adi Roiban
On Sep 30, 2017, at 3:14 PM, Adi Roiban <adi@roiban.ro> wrote:
I am restarting this discussion https://twistedmatrix.com/pipermail/twisted-python/2016-May/030333.html
I am starting a new thread since I want to keep the focus on the review process / workflow / markers, and not on the things required to accept a PR or do a review.
I'm a little confused. What do you mean by "process / workflow / markers" if not "the things required to accept a PR or do a review"?
----------
Proposing: Just open a pull request. Any open pull request should be treated as part of the queue.
I don't like this. If you are not a comitter, you need to open a PR to trigger the tests.
So you want to first open a PR, then wait for tests to execute, then fix and only after that to request the review.
We can start with setting the title to have "[WIP]" marker, to let others know that this is not yet ready... but then when changes are required, the reviewer will have to set the WIP marker again.. and if the reviewer is not a team member, it will not have rights to edit the subject.
But I hope that we can have a bot which once a "please review" comment is left, it will set a label.
Mark Williams already started working on this: https://github.com/markrwilliams/txghbot
Accepting: A committer pushes the big green button;
+1 ... but maybe also leave a comment :)
We should have more verbiage around the comment stuff, including the "always say thank you twice" rule which I don't think is written down anywhere :-).
Reviewing: This is the potentially slightly odd part. I believe a review that doesn't result in acceptance should close the PR. We need to be careful to always include some text that explains that closing a PR does not mean that the change is rejected, just that the submitter must re-submit. Initially this would just mean opening a new PR, but Mark Williams is working on a bot to re-open pull requests when a submitter posts a "please review" comment: https://github.com/markrwilliams/txghbot
Since we will have a bot for "please review", why not use the same bot to set a label on "please make changes" ?
Yep!
I think that closing a PR should mean that the work on that branch is rejected :)
I still want to require people to open an issue first so we can separate closing PRs ("this patch is too bad to be accepted, please try again") from closing tickets or issues ("we do not want to do this work").
Responding: A submitter can open a new PR, or, once we start running txghbot, reopen their closed PR.
As commented above, I am +1 for leaving a "please review" comment and having a bot updating the labels.
Viewing: https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-status%3Afailed
One we get the "please-review" and "changes-needed" labels it should be eaiser to view the queue.
I can update https://twisted.reviews/ <https://twisted.reviews/> (at some point) when to point at the appropriate link. (You've all got it bookmarked, right?)
-------
Whem multiple reviewers are required, you can use the dedicated GitHub Review message and approve it without hiting the merge button.
---------
I have no idea how other projects are managing the review queue.
"Poorly" :-). A failure mode of many open source projects is that they have terrible latency when responding to new contributors.
Please send your feedback.
If we agree on a process based on managing the labes, I can work on implemeting the required logic with a bot and GitHub hooks.
Please do coordinate with Mark on this, if you weren't already ;-).
--------
We can also start by using the WIP marker
* while preparing the PR * once changes are required and the author works on addressing the changes requsted on review
Any PR which is open and does not have the WIP marker means that is part of the queue.
----------
Thanks!
PS: I have checked pyca/crypography but I don't see any pattern there and a lot of PR are merged without any comment https://github.com/pyca/cryptography/pulls?q=is%3Apr+is%3Aclosed <https://github.com/pyca/cryptography/pulls?q=is:pr+is:closed>
Cryptography is kind of a weird project with a somewhat rarified contributor audience, I don't know if it makes sense to copy their process. -g
On Sat, Sep 30, 2017 at 3:14 PM, Adi Roiban <adi@roiban.ro> wrote:
orking on a bot to re-open pull requests when a submitter posts a "please review" comment: https://github.com/markrwilliams/txghbot
Rather than write a bot specific to Twisted, why not just adapt the Twisted project to use a bot written by another project? For example, the Zulip project has written zulipbot: https://github.com/zulip/zulipbot#zulipbot Docs for it are here: http://zulip.readthedocs.io/en/latest/zulipbot-usage.html and a presentation for it was given here: http://opensourcebridge.org/sessions/1992 A lot of the motivations for zulipbot are similar to what the Twisted project is facing: https://github.com/zulip/zulipbot/wiki/FAQ such as giving non-committers the ability to add labels to issues or PR's. zulipbot is actively used, and you can see it in action in their issues and PR's: https://github.com/zulip/zulip/issues?utf8=%E2%9C%93&q=is%3Aissue%20or%20is%3Apull I've met some of the Zulip developers at Pybay. They are really focused on improving developer experience and onboarding of new developers. Rather than maintain our own bot, the Twisted project would probably get further along with zulipbot, and working with the zulip developers to fill in any missing gaps on that bot. -- Craig
On Oct 1, 2017, at 12:06 PM, Craig Rodrigues <rodrigc@crodrigues.org> wrote:
Rather than write a bot specific to Twisted, why not just adapt the Twisted project to use a bot written by another project?
Personally I don't care either way :-). I was just describing my understanding of the existing plan, not endorsing it. Hopefully someone more directly involved will comment. -g
On 1 October 2017 at 20:09, Glyph <glyph@twistedmatrix.com> wrote:
On Oct 1, 2017, at 12:06 PM, Craig Rodrigues <rodrigc@crodrigues.org> wrote:
Rather than write a bot specific to Twisted, why not just adapt the Twisted project to use a bot written by another project?
Personally I don't care either way :-). I was just describing my understanding of the existing plan, not endorsing it. Hopefully someone more directly involved will comment.
I expect that in order to make this happen, we will need a transition period in which the current system should continue to work, that is automatically set/remove "review" keyword to Trac. In this way, the new way(tm) can be implemented in various ways (and experiment with different ways) and nobody should complain as the old system will just work. For my project, I am using Klein as the hooks server, but it is based on Trac XML-RPC, so I don't think it can be used for Twisted. --------------- With that in mind, I think that txghbot or something based on python/twisted is easier. We would need to talk with Trac... maybe over PB I remember there was a PB based channel for communicating with Trac but I can't find where it is defined. I see that kenan is using something [1] BOT_PORT = 15243, I can't find where a service with that port is started. Regards, Adi [1] https://github.com/twisted-infra/braid/blob/3dc2fea9a29c1b8361961d561fea1fc5...
On Oct 2, 2017, at 2:48 AM, Adi Roiban <adi@roiban.ro> wrote:
On 1 October 2017 at 20:09, Glyph <glyph@twistedmatrix.com> wrote:
On Oct 1, 2017, at 12:06 PM, Craig Rodrigues <rodrigc@crodrigues.org> wrote:
Rather than write a bot specific to Twisted, why not just adapt the Twisted project to use a bot written by another project?
Personally I don't care either way :-). I was just describing my understanding of the existing plan, not endorsing it. Hopefully someone more directly involved will comment.
I expect that in order to make this happen, we will need a transition period in which the current system should continue to work, that is automatically set/remove "review" keyword to Trac. In this way, the new way(tm) can be implemented in various ways (and experiment with different ways) and nobody should complain as the old system will just work.
For my project, I am using Klein as the hooks server, but it is based on Trac XML-RPC, so I don't think it can be used for Twisted.
---------------
With that in mind, I think that txghbot or something based on python/twisted is easier. We would need to talk with Trac... maybe over PB
I remember there was a PB based channel for communicating with Trac but I can't find where it is defined. I see that kenan is using something [1] BOT_PORT = 15243, I can't find where a service with that port is started.
The PB interface is unmaintained and pretty much only still exists for IRC notifications (if I remember correctly); the process binding that port is Kenaan (the IRC bot). https://trac-hacks.org/wiki/XmlRpcPlugin <https://trac-hacks.org/wiki/XmlRpcPlugin> is now a bit of a misnomer as it also supports JSON-RPC as well. We should probably just check that the plugin is upgraded and use that. -glyph
On 3 October 2017 at 00:09, Glyph <glyph@twistedmatrix.com> wrote:
[snip]
I remember there was a PB based channel for communicating with Trac but I can't find where it is defined. I see that kenan is using something [1] BOT_PORT = 15243, I can't find where a service with that port is started.
The PB interface is unmaintained and pretty much only still exists for IRC notifications (if I remember correctly); the process binding that port is Kenaan (the IRC bot).
https://trac-hacks.org/wiki/XmlRpcPlugin is now a bit of a misnomer as it also supports JSON-RPC as well. We should probably just check that the plugin is upgraded and use that.
-glyph
Thanks for the info. With XML/JSON RPC I should be able to get something. -------- I have updated the github bot for my project to do GitHub label management. I am using a slightly different Trac process than Twisted. As a proof of concept, I can set the bot to be used for twisted-infra/braid as for Braid we don't need to sync with Trac. We can use it to get a hands-on experience on the label management part, and once we agree on how it should behave, I can look at deploying a dedicated bot for Twisted and then update it to sync with Trac. What do you think? PS: I am not deploying it into Twisted infra now as for me twisted-infra/braid and vagrant vm is broken... working on fixing it -- Adi Roiban
participants (3)
-
Adi Roiban -
Craig Rodrigues -
Glyph