[Twisted-Python] Responding to PRs

[Itamar Turner-Trauring]

Hi all,

Since we're starting to get PRs from random people it's worth trying to
make the process as friendly as possible.

So, maybe instead of telling new contribtutors "PRs won't be reviewed
without an issue, see contributor guidelines" it would be better to say
"Thanks for the PR! I opened an issue for this PR here (tm.tl/12345). In
the future we'd appreciate it if you could open an issue before
submitting PRs; see contributor guidelines for details".

If someone went out of their way to provide a fix, we should try to
minimize any unnecessary stop-energy they encounter along the way, even
if that means some people won't learn the intricacies of the process.

-Itamar

[Craig Rodrigues]

Hi,

I support this approach.

I have a few small suggestions.
(1) Give the exact link of some reasonable text that you want people to
read. The contributor guidelines are accurate,
but the text is verbose. It takes a while to get to the text that
gives you the actual steps to follow.

(2) GitHub uses the term "issue", while Trac uses the term "ticket". If
you tell people to open an "issue" they
might get confused and try to open a GitHub issue.

*"Thanks for the PR! I opened a ticket for this PR here (tm.tl/12345
<http://tm.tl/12345>). In the future we'd appreciate it if you could open a
ticket before submitting PRs;
see https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch
<https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch> "*

If the Twisted developers get very efficient at opening Trac tickets on
behalf of patch submitters,
then the incentive for patch submitters to interact with Trac goes down.
:) :)

--
Craig

Post by Itamar Turner-Trauring
Hi all,
Since we're starting to get PRs from random people it's worth trying to
make the process as friendly as possible.
So, maybe instead of telling new contribtutors "PRs won't be reviewed
without an issue, see contributor guidelines" it would be better to say
"Thanks for the PR! I opened an issue for this PR here (tm.tl/12345). In
the future we'd appreciate it if you could open an issue before submitting
PRs; see contributor guidelines for details".
If someone went out of their way to provide a fix, we should try to
minimize any unnecessary stop-energy they encounter along the way, even if
that means some people won't learn the intricacies of the process.
-Itamar

[Adi Roiban]

Post by Itamar Turner-Trauring
Hi all,
Since we're starting to get PRs from random people it's worth trying to
make the process as friendly as possible.
So, maybe instead of telling new contribtutors "PRs won't be reviewed
without an issue, see contributor guidelines" it would be better to say
"Thanks for the PR! I opened an issue for this PR here (tm.tl/12345). In
the future we'd appreciate it if you could open an issue before submitting
PRs; see contributor guidelines for details".
If someone went out of their way to provide a fix, we should try to
minimize any unnecessary stop-energy they encounter along the way, even if
that means some people won't learn the intricacies of the process.

Beside the fix, we need tests, documentation, release notes fragment....
sometimes compatibility with python 2 and python3... so I would say that
Twisted is not designed for low-energy contributors.

I would like to thank Craig for monitoring the Twisted PR. For low-energy
dudes, like me, it is of great help as I am only monitoring the Twisted's
official review queue (https://twistedmatrix.com/trac/report/25) which is
already huge.

Thanks again Craig and keep up the good work ! :)

[Glyph]

Post by Itamar Turner-Trauring
Hi all,
Since we're starting to get PRs from random people it's worth trying to make the process as friendly as possible.
So, maybe instead of telling new contribtutors "PRs won't be reviewed without an issue, see contributor guidelines" it would be better to say "Thanks for the PR! I opened an issue for this PR here (tm.tl/12345). In the future we'd appreciate it if you could open an issue before submitting PRs; see contributor guidelines for details".

Anyone who wants to do this is absolutely welcome to. Personally, I won't be, but just because there's more than enough work for me to do on the "official" review queue if I have time for it, and manually de-duplicating all the data is challenging.

Post by Itamar Turner-Trauring
If someone went out of their way to provide a fix, we should try to minimize any unnecessary stop-energy they encounter along the way, even if that means some people won't learn the intricacies of the process.

I don't have any interest in teaching people the intricacies of this somewhat janky process :-). The sooner we can switch to the "review queue" simply being open PRs, the better; so thanks for volunteering to manage the correspondence in the meanwhile.

-glyph

[Adi Roiban]

On 13 June 2016 at 03:09, Glyph <***@twistedmatrix.com> wrote:

[snip]

I don't have any interest in teaching people the intricacies of this

Post by Glyph
somewhat janky process :-). The sooner we can switch to the "review queue"
simply being open PRs, the better; so thanks for volunteering to manage the
correspondence in the meanwhile.

Is there a wiki page, a ticket or some place where switching to the 'review
queue' as GitHub PR list is discussed or brainstormed?

I could not find any reference in the git migration plan
https://github.com/twisted-infra/braid/milestones/Migrate-to-Git

[Glyph]

Is there a wiki page, a ticket or some place where switching to the 'review queue' as GitHub PR list is discussed or brainstormed?

Thus far all discussion has been on the mailing list. I feel like putting it on the wiki would not be that useful, though; hopefully the discussion will continue for at most another month or two, and it's mostly just a question of coming to consensus about how exactly we're going to use the queue and model discrete review/resubmit events than doing a bunch of work. https://github.com/markrwilliams/txghbot is probably most of what we need already, perhaps with one or two slight tweaks?

-glyph

[Mark Williams]

Post by Glyph
Thus far all discussion has been on the mailing list. I feel like putting it on the wiki would not be that useful, though; hopefully the discussion will continue for at most another month or two, and it's mostly just a question of coming to consensus about how exactly we're going to use the queue and model discrete review/resubmit events than doing a bunch of work. https://github.com/markrwilliams/txghbot is probably most of what we need already, perhaps with one or two slight tweaks?

I'm the owner of txghbot. I hope it ends up being useful for Twisted!

Despite the stern warning at the top of the README, the process it describes should result in a functioning GitHub bot.

If you feel adventurous you can write your own webhooks. They're Twisted plugins that implement this interface:

https://github.com/markrwilliams/txghbot/blob/master/txghbot/_core.py#L42-L79

I'll claim the lack of any abstraction over the GitHub Webhook API is intentional; this remains the authoritative documentation:

https://developer.github.com/webhooks/

Please let me know if there's anything I can do to make txghbot make PRs easier for everyone. If it ends up being at all useful I'm happy to transfer ownership to the Twisted organization.

-Mark

[Glyph]

Post by Mark Williams

Post by Glyph
Thus far all discussion has been on the mailing list. I feel like putting it on the wiki would not be that useful, though; hopefully the discussion will continue for at most another month or two, and it's mostly just a question of coming to consensus about how exactly we're going to use the queue and model discrete review/resubmit events than doing a bunch of work. https://github.com/markrwilliams/txghbot is probably most of what we need already, perhaps with one or two slight tweaks?

I'm the owner of txghbot. I hope it ends up being useful for Twisted!

I strongly suspect that it will be the official solution. Thanks so much for doing this - the existence of this code is a structural expression of the setup process which short-circuits me needing to read and process all the developer documentation ;).

Post by Mark Williams
Despite the stern warning at the top of the README, the process it describes should result in a functioning GitHub bot.

Cool. I will set that up soon.

Post by Mark Williams
https://github.com/markrwilliams/txghbot/blob/master/txghbot/_core.py#L42-L79
https://developer.github.com/webhooks/
Please let me know if there's anything I can do to make txghbot make PRs easier for everyone. If it ends up being at all useful I'm happy to transfer ownership to the Twisted organization.

The first thing that comes to mind is that you could get Tom Prince to give you write access to txghbot on PyPI so that this could be 'pip install'ed like anything else, instead of cut live from ***@HEAD :-).

I think that some folks were really over-focused on the whole "closing PRs" part of the previous discussion, when what I was really trying to get at was the need for a single, clear "review queue". Something like txghbot is necessary no matter how we do it because non-(maintainer|reviewer|person with repo:write access|we need a good standard word for this)s will not be able to manipulate the labels.

At this point I think closing PRs creates more problems than it solves. In particular:

It means that people can't push changes to their PRs to experiment with travis build status, because reviewers will keep closing them, which prevents further pushes from running in CI.
It means that force-pushing has weird and confusing side-effects (even weirder and confusinger than usual). I am not a big fan of the rebase/force-push workflow, but judiciously used (i.e. with interactive rebases) it can help with splitting up big changes, and it also mitigates Github's atrocious management of the diff display of long-running branches.
It is definitely perceived as an "abnormal" way of doing reviews on Github.

Point 3 was not enough to dissuade me, but it certainly isn't a point in favor, and in combination with the other two I don't think it looks good.

So, instead of treating /pulls as our review queue, I think something like <https://github.com/twisted/twisted/pulls?utf8=✓&q=is%3Apr+is%3Aopen+-label%3A%22awaiting+review+response%22+-status%3Afailure> will have to do. By subtracting a label from the review query rather than adding one, we can make it so that if our bot breaks down, contributors can still get their new submissions reviewed, and in the worst case where it is down when they want to resubmit, they can work around a broken bot by closing their "awaiting" PR and opening a new one. (The ability to work around broken infra is important because I have to assume that things are going to go wrong with this, it being a distributed system on the public Internet.)

This means that txghbot's responsibility, instead of reopening the PR, will be to add and remove the 'awaiting review response' label. If you wanted to write the actual plugin for doing that it might be helpful. And then setting up a repo where we can play with it to test it out before turning it on for twisted/twisted :).

-glyph

[Adi Roiban]

Post by Mark Williams
I'm the owner of txghbot. I hope it ends up being useful for Twisted!
I strongly suspect that it will be the official solution. Thanks so much
for doing this - the existence of this code is a structural expression of
the setup process which short-circuits me needing to read and process all
the developer documentation ;).

I am trying to work on GitHub <-> Buildbot intergration (2 ways) and for
doing this we will end up with something like a "bot" running
inside/alongside Buildbot and triggering builds based on webhooks.

We will also have another "bot" inside buildbot which will POST Commit
Status requests to GitHub.

Now, my questions are:

* Do we need an external txghbot ?
* Can we integrate all the webhook logic inside the buildbot hook handler
or inside the txghbot?

By integrating all webhooks handlers in a single place, for me it will be
easier to discover everything which is linked of triggered by webhooks.

Cheers,
Adi

PS: GitHub <-> Buildbot integration will follow soon after this PR will be
reviewed and approved https://github.com/twisted-infra/braid/pull/205 :)

[Glyph]

Post by Glyph

Post by Mark Williams
I'm the owner of txghbot. I hope it ends up being useful for Twisted!

I strongly suspect that it will be the official solution. Thanks so much for doing this - the existence of this code is a structural expression of the setup process which short-circuits me needing to read and process all the developer documentation ;).
I am trying to work on GitHub <-> Buildbot intergration (2 ways) and for doing this we will end up with something like a "bot" running inside/alongside Buildbot and triggering builds based on webhooks.
We will also have another "bot" inside buildbot which will POST Commit Status requests to GitHub.
* Do we need an external txghbot ?
* Can we integrate all the webhook logic inside the buildbot hook handler or inside the txghbot?
By integrating all webhooks handlers in a single place, for me it will be easier to discover everything which is linked of triggered by webhooks.
Cheers,
Adi
PS: GitHub <-> Buildbot integration will follow soon after this PR will be reviewed and approved https://github.com/twisted-infra/braid/pull/205 <https://github.com/twisted-infra/braid/pull/205> :)

I'd rather have a separate bot for workflow stuff, because buildbot can be fairly opaque and difficult to understand or run in isolation, especially if someone wants to set it up against a test project to try out workflow changes.

-glyph

[Adi Roiban]

Post by Adi Roiban
Is there a wiki page, a ticket or some place where switching to the
'review queue' as GitHub PR list is discussed or brainstormed?
Thus far all discussion has been on the mailing list. I feel like putting
it on the wiki would not be that useful, though; hopefully the discussion
will continue for at most another month or two, and it's mostly just a
question of coming to consensus about how exactly we're going to use the
queue and model discrete review/resubmit events than doing a bunch of work.
https://github.com/markrwilliams/txghbot is probably most of what we
need already, perhaps with one or two slight tweaks?

OK. Thanks!

I was not sure if the github bot technical decision was taken.

I was not sure if we made a decision for labels vs PR state.

I am -1 for closing a PR as a way of saying: changes are pretty good but a
few tests are missing :)

---------

I am +1 on using labels/keywords in GitHub but I am not sure what will be
the relation between the Trac tickets and the PRs?
Will there be links between them?
Will the Trac tickets be migrated to GitHub Issues?

---------

In case it helps, for my project I am doing it in another way.

I am not suggesting that Twisted should use the same process, but maybe it
can help to get an idea of how this can be done.

The code for the bot is here https://github.com/chevah/github-hooks-server
... and it uses Trac via xml-rpc

The tickets are hosted in Trac, but I have implemented a Trac ticket
workflow. I can share the Trac configuration.

Instead of adding the 'review' keyword, a ticket is actually placed in a
'need_review' state... and instead of removing the 'review' keyword and
assigning the ticket back the ticket can either go into a
'changes_approved' state ... or in a 'need_more_work' state.

Each PR has an associated ticket in Trac.

I have 'needs-review', 'changes-approved' and 'needs-changes' keyword for
managing the ticket state via GitHub PR comments.
The bot also update the "branch" Trac field with a link to the latest PR
associated with a Trac ticket

You can see an example here for the GitHub side
https://github.com/chevah/python-package/pull/54 ... the Trac side is
private but I can share a screenshot if required.

The bot is briefly described here
http://styleguide.chevah.com/review.html#overview-of-the-github-and-trac-integration

Regards,

[Adi Roiban]

On 12 June 2016 at 21:32, Itamar Turner-Trauring <***@itamarst.org>
wrote:

[snip]

Post by Itamar Turner-Trauring
So, maybe instead of telling new contribtutors "PRs won't be reviewed
without an issue, see contributor guidelines" it would be better to say
"Thanks for the PR! I opened an issue for this PR here (tm.tl/12345). In
the future we'd appreciate it if you could open an issue before submitting
PRs; see contributor guidelines for details".

[snip]

I think that is important that the Trac ticket is created by the PR
contributor. In this way we can make sure that the PR contributor has an
account in Trac and that the contributor will receive notifications for all
the discussions done in the Trac ticket.

AFAIK the PR are just replacing the attached patched to a Trac ticket. The
review should still be done in the Trac ticket.

Cheers,