Re: [Twisted-Python] "Cleared to Land"?

[Adi Roiban]

On Sat, 7 Jul 2018 at 22:45, Glyph  wrote:
>
>
>
> On Jul 7, 2018, at 12:36 PM, Tom Most  wrote:
>

[snip]

> Thanks for driving this to completion, Tom!  Much appreciated.
>
> For future reference though, when landing stuff on trunk, please use this 
> format exactly:
>
> https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechangetotrunk
>
> This helps with both automatically closing the associated Trac ticket (I 
> already did that in this case) and properly attributing credit for reviewer 
> points on https://twistedmatrix.com/highscores/.

I always forget the format and then I have to search previous commits or wiki.

We make reference to the Trac ticket in PR title, Branch Name, PR
description (As part of checklist).

Do we really need to make another reference in the merge commit?
For me, it's red tape.

I don't think that we should make things more complicated, just to
have a functional https://twistedmatrix.com/highscores/

> Perhaps we should avoid the "cleared to land" label on PRs from 
> non-committers? I scan through open PRs for ones which require a procedural 
> nudge now and again, but I had not looked at this one as the process appeared 
> to be done with it, and I missed that it was from a non-committer.
>
>
> Ideally it would be used sparingly, but, the availability of such a process 
> release-valve allows someone to do a review even if they only have time to 
> read the code, and not the time to sit around waiting 40 minutes for some 
> intermittent CI nonsense to shake out.
>
> Since this doesn't have a https://twisted.reviews/ -like "core gameplay loop" 
> that project members regularly visit, perhaps if you're going to use this 
> label you should always shoot a message to this list as well, to let other 
> contributors know that they should take a look if they have a minute?
>
> On that note, it looks like https://github.com/twisted/twisted/pull/1010 has 
> suffered the same fate?  Any other committers have a minute to land that one? 
> :)

https://github.com/twisted/twisted/pull/1010 was merged now.

Do we really need and up to date branch before a merge?

I was thinking that with the "clear to land" label, we can have a
robot which checks for the PR and once all tests are green, the robot
will automatically merge based on
https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button

The robot can also auto-update with 'trunk/master' a PR with clear to land.

I am happy to work at automation and improving the Twisted PR process.

For example, I have this PR https://github.com/twisted/twisted/pull/1011

It only touches  .circleci/config.yml  why should we block or wait
for the merge/review of this PR due to random failures on Buildbot?

-

BTW. Do we need to run the documentation tests on Buildbot?
There are some documentation tests on Circle-CI. Is that not enough ?
https://circleci.com/gh/twisted/twisted/1215

-- 
Adi Roiban

___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] "Cleared to Land"?

[Glyph]

> On Jul 8, 2018, at 7:44 AM, Adi Roiban  wrote:
> 
> On Sat, 7 Jul 2018 at 22:45, Glyph  wrote:
>> 
>> 
>> 
>> On Jul 7, 2018, at 12:36 PM, Tom Most  wrote:
>> 
> 
> [snip]
> 
>> Thanks for driving this to completion, Tom!  Much appreciated.
>> 
>> For future reference though, when landing stuff on trunk, please use this 
>> format exactly:
>> 
>> https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechangetotrunk
>> 
>> This helps with both automatically closing the associated Trac ticket (I 
>> already did that in this case) and properly attributing credit for reviewer 
>> points on https://twistedmatrix.com/highscores/.
> 
> I always forget the format and then I have to search previous commits or wiki.
> 
> We make reference to the Trac ticket in PR title, Branch Name, PR
> description (As part of checklist).
> 
> Do we really need to make another reference in the merge commit?
> For me, it's red tape.
> 
> I don't think that we should make things more complicated, just to
> have a functional https://twistedmatrix.com/highscores/ 
> 

If we forget every so often, it's not a big deal, but I find it's nice to have 
a generally consistent merge format.  I do really wish that Github could do 
this in the same way that it does for a PR template so we didn't have to 
remember manually.

>> Perhaps we should avoid the "cleared to land" label on PRs from 
>> non-committers? I scan through open PRs for ones which require a procedural 
>> nudge now and again, but I had not looked at this one as the process 
>> appeared to be done with it, and I missed that it was from a non-committer.
>> 
>> 
>> Ideally it would be used sparingly, but, the availability of such a process 
>> release-valve allows someone to do a review even if they only have time to 
>> read the code, and not the time to sit around waiting 40 minutes for some 
>> intermittent CI nonsense to shake out.
>> 
>> Since this doesn't have a https://twisted.reviews/ -like "core gameplay 
>> loop" that project members regularly visit, perhaps if you're going to use 
>> this label you should always shoot a message to this list as well, to let 
>> other contributors know that they should take a look if they have a minute?
>> 
>> On that note, it looks like https://github.com/twisted/twisted/pull/1010 has 
>> suffered the same fate?  Any other committers have a minute to land that 
>> one? :)
> 
> https://github.com/twisted/twisted/pull/1010 was merged now.
> 
> Do we really need and up to date branch before a merge?
> 
> I was thinking that with the "clear to land" label, we can have a
> robot which checks for the PR and once all tests are green, the robot
> will automatically merge based on
> https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button 
> 

This sounds like a great idea, and is very close to what I would do if I had 
time for working on automation :).

Rather than using the "clear to land" label, however, I'd probably do this on 
the basis of a passing review from a project member being present, and a 
passing build.  (The "clear to land" convention predates Github native code 
reviews.)

> The robot can also auto-update with 'trunk/master' a PR with clear to land.
> 
> I am happy to work at automation and improving the Twisted PR process.

I know you're not a huge fan of Trac either - it would be a nice touch if the 
robot here could also notice Github reviews and delete the 'review' keyword; 
this would keep our highscores and stats stuff working, but eliminate a manual 
step for reviewers :).

> For example, I have this PR https://github.com/twisted/twisted/pull/1011
> 
> It only touches  .circleci/config.yml  why should we block or wait
> for the merge/review of this PR due to random failures on Buildbot?

It looks like this is another case of  
twisted.python.test.test_sendmsg.CModuleSendmsgTests.test_shortsend messing up 
our build results.

> BTW. Do we need to run the documentation tests on Buildbot?
> There are some documentation tests on Circle-CI. Is that not enough ?
> https://circleci.com/gh/twisted/twisted/1215 
> 

I don't think that this should be a required check on buildbot; we definitely 
shouldn't block on it, especially for bugfixes, but one of the advantages of 
the buildbot check as opposed to the CircleCI/Travis checks is that the 
Buildbot check uploads an artifact.  If you look at 
https://buildbot.twistedmatrix.com/builders/documentation/builds/4071 
 for 
example, you can see that the .tar.bz2 files under the last 2 steps are 
downloadable, and you can crack them open to look at the index.html inside and 
see what the built docs actually look like.  Sadly we haven't had a lot of doc 
PRs lately, but I do use this occasionally.

If we can host built arti

Re: [Twisted-Python] "Cleared to Land"?

[Tom Most]

On Sun, Jul 8, 2018, at 12:06 PM, Glyph wrote:
>> On Jul 8, 2018, at 7:44 AM, Adi Roiban  wrote:
>> 
>> On Sat, 7 Jul 2018 at 22:45, Glyph  wrote:
>>> On Jul 7, 2018, at 12:36 PM, Tom Most  wrote:
>> 
>> [snip]
>> 
>>> Thanks for driving this to completion, Tom!  Much appreciated.
>>> 
>>> For future reference though, when landing stuff on trunk, please use
>>> this format exactly:>>> 
>>> https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechangetotrunk>>>
>>>  
>>> This helps with both automatically closing the associated Trac
>>> ticket (I already did that in this case) and properly attributing
>>> credit for reviewer points on https://twistedmatrix.com/highscores/.>> 
>> I always forget the format and then I have to search previous commits
>> or wiki.>> 
>> We make reference to the Trac ticket in PR title, Branch Name, PR
>> description (As part of checklist).
>> 
>> Do we really need to make another reference in the merge commit?
>> For me, it's red tape.
>> 
>> I don't think that we should make things more complicated, just to
>> have a functional https://twistedmatrix.com/highscores/
> 
> If we forget every so often, it's not a big deal, but I find it's nice
> to have a generally consistent merge format.  I do really wish that
> Github could do this in the same way that it does for a PR template so
> we didn't have to remember manually.
Ah, sorry. It looks like I've done this a few times. I had wondered why
my points weren't changing.
A template would be nice, if only as a reminder, but I think that I will
always struggle with this if I use the GitHub UI to merge --- no other
project to which I contribute has requirements w.r.t. the merge commit
message, so I am fighting habit. I'll look into scripting it.
As an aside, could you update "" in the merge
commit message example to ""? I always
have to look at the repository history to see if this is supposed to be
full names or Trac/GitHub handles.
>>> Perhaps we should avoid the "cleared to land" label on PRs from non-
>>> committers? I scan through open PRs for ones which require a
>>> procedural nudge now and again, but I had not looked at this one as
>>> the process appeared to be done with it, and I missed that it was
>>> from a non-committer.>>> 
>>> 
>>> Ideally it would be used sparingly, but, the availability of such a
>>> process release-valve allows someone to do a review even if they
>>> only have time to read the code, and not the time to sit around
>>> waiting 40 minutes for some intermittent CI nonsense to shake out.>>> 
>>> Since this doesn't have a https://twisted.reviews/ -like "core
>>> gameplay loop" that project members regularly visit, perhaps if
>>> you're going to use this label you should always shoot a message to
>>> this list as well, to let other contributors know that they should
>>> take a look if they have a minute?>>> 
>>> On that note, it looks like
>>> https://github.com/twisted/twisted/pull/1010 has suffered the same
>>> fate?  Any other committers have a minute to land that one? :)>> 
>> https://github.com/twisted/twisted/pull/1010 was merged now.
>> 
>> Do we really need and up to date branch before a merge?

The GitHub UI currently requires this for non-admins, though it can be
bypassed with a manual merge, much like the requirement for a green
build. As a new committer I haven't felt comfortable doing that bypass,
so I spend a lot of time merging forward and kicking builds to get a
green check mark. Should I be bothering? Particularly for documentation
changes[1]. I'd rather spend that time on work product than process.
>> I was thinking that with the "clear to land" label, we can have a
>> robot which checks for the PR and once all tests are green, the robot>> will 
>> automatically merge based on
>> https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button> 
> This sounds like a great idea, and is very close to what I would do if
> I had time for working on automation :).> 
> Rather than using the "clear to land" label, however, I'd probably do
> this on the basis of a passing review from a project member being
> present, and a passing build.  (The "clear to land" convention
> predates Github native code reviews.)
I think we'd still need a label or other explicit indicator that an
automatic merge should occur, or we won't be able to leave a passing
review which suggests trivial fixes. "Please fix this typo, then merge"
reviews are pretty common.
>> The robot can also auto-update with 'trunk/master' a PR with clear
>> to land.>> 
>> I am happy to work at automation and improving the Twisted PR
>> process.> 
> I know you're not a huge fan of Trac either - it would be a nice touch
> if the robot here could also notice Github reviews and delete the
> 'review' keyword; this would keep our highscores and stats stuff
> working, but eliminate a manual step for reviewers :).
Yes, let's also add/remove remove the "review required" label on GitHub
or it ends up[2] out of sync[3]. :)
Have yo

Re: [Twisted-Python] "Cleared to Land"?

[Tom Most]

On Sun, Jul 8, 2018, at 6:21 PM, Tom Most wrote:
> The GitHub UI currently requires this for non-admins, though it can be
> bypassed with a manual merge, much like the requirement for a green
> build. As a new committer I haven't felt comfortable doing that
> bypass, so I spend a lot of time merging forward and kicking builds to
> get a green check mark. Should I be bothering? Particularly for
> documentation changes[1]. I'd rather spend that time on work product
> than process.
Please disregard this. I tried it on #1037[2] but branch protection
still applies:
$ git push origin trunk
Counting objects: 6, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (5/5), done.
Writing objects: 100% (6/6), 610 bytes | 0 bytes/s, done.
Total 6 (delta 4), reused 0 (delta 0)
remote: Resolving deltas: 100% (4/4), completed with 4 local objects.
remote: error: GH006: Protected branch update failed for 
refs/heads/trunk.remote: error: 3 of 3 required status checks are expected.
To g...@github.com:twisted/twisted.git
! [remote rejected] trunk -> trunk (protected branch hook declined)
error: failed to push some refs to 'g...@github.com:twisted/twisted.git'
---Tom

Links:

  1. https://github.com/twisted/twisted/pull/1037
  2. https://github.com/twisted/twisted/pull/1037
___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] "Cleared to Land"?

[Glyph]

> On Jul 8, 2018, at 6:21 PM, Tom Most  wrote:
> 
> On Sun, Jul 8, 2018, at 12:06 PM, Glyph wrote:
>>> On Jul 8, 2018, at 7:44 AM, Adi Roiban >> > wrote:
>>> 
>>> On Sat, 7 Jul 2018 at 22:45, Glyph >> > wrote:
 On Jul 7, 2018, at 12:36 PM, Tom Most >>> > wrote:
>>> 
>>> [snip]
>>> 
 Thanks for driving this to completion, Tom!  Much appreciated.
 
 For future reference though, when landing stuff on trunk, please use this 
 format exactly:
 
 https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechangetotrunk
  
 
 
 This helps with both automatically closing the associated Trac ticket (I 
 already did that in this case) and properly attributing credit for 
 reviewer points on https://twistedmatrix.com/highscores/ 
 .
>>> 
>>> I always forget the format and then I have to search previous commits or 
>>> wiki.
>>> 
>>> We make reference to the Trac ticket in PR title, Branch Name, PR
>>> description (As part of checklist).
>>> 
>>> Do we really need to make another reference in the merge commit?
>>> For me, it's red tape.
>>> 
>>> I don't think that we should make things more complicated, just to
>>> have a functional https://twistedmatrix.com/highscores/ 
>>> 
>> 
>> If we forget every so often, it's not a big deal, but I find it's nice to 
>> have a generally consistent merge format.  I do really wish that Github 
>> could do this in the same way that it does for a PR template so we didn't 
>> have to remember manually.
> 
> Ah, sorry. It looks like I've done this a few times. I had wondered why my 
> points weren't changing.
> 
> A template would be nice, if only as a reminder, but I think that I will 
> always struggle with this if I use the GitHub UI to merge --- no other 
> project to which I contribute has requirements w.r.t. the merge commit 
> message, so I am fighting habit. I'll look into scripting it.
> 
> As an aside, could you update "" in the merge commit 
> message example to ""? I always have to 
> look at the repository history to see if this is supposed to be full names or 
> Trac/GitHub handles.

I've just made you a trac admin, so you should be able to have at it :-).

 Perhaps we should avoid the "cleared to land" label on PRs from 
 non-committers? I scan through open PRs for ones which require a 
 procedural nudge now and again, but I had not looked at this one as the 
 process appeared to be done with it, and I missed that it was from a 
 non-committer.
 
 
 Ideally it would be used sparingly, but, the availability of such a 
 process release-valve allows someone to do a review even if they only have 
 time to read the code, and not the time to sit around waiting 40 minutes 
 for some intermittent CI nonsense to shake out.
 
 Since this doesn't have a https://twisted.reviews/ 
  -like "core gameplay loop" that project members 
 regularly visit, perhaps if you're going to use this label you should 
 always shoot a message to this list as well, to let other contributors 
 know that they should take a look if they have a minute?
 
 On that note, it looks like https://github.com/twisted/twisted/pull/1010 
  has suffered the same fate? 
  Any other committers have a minute to land that one? :)
>>> 
>>> https://github.com/twisted/twisted/pull/1010 
>>>  was merged now.
>>> 
>>> Do we really need and up to date branch before a merge?
> 
> The GitHub UI currently requires this for non-admins, though it can be 
> bypassed with a manual merge, much like the requirement for a green build. As 
> a new committer I haven't felt comfortable doing that bypass, so I spend a 
> lot of time merging forward and kicking builds to get a green check mark. 
> Should I be bothering? Particularly for documentation changes 
> . I'd rather spend that time on 
> work product than process.

On the one hand it saddens me that contributors are having their time wasted by 
this pointless pacifying of CI machines.  On the other hand, to echo your 
sentiment of "buggy tests are worse than no tests", I think that there's no 
point to having CI at all unless we're paying attention to it, so I would 
wholeheartedly support the deletion or skipping of any tests that are causing 
real problems.

>>> I was thinking that with the "clear to land" label, we can have a
>>> robot which checks for the PR and once all tests are green, the robot
>>> will automatically merge based on
>>> https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button 
>>>