[Twisted-Python] "Cleared to Land"?

[Glyph]

> On Jul 8, 2018, at 7:44 AM, Adi Roiban <adi at roiban.ro> wrote:
> 
> On Sat, 7 Jul 2018 at 22:45, Glyph <glyph at twistedmatrix.com> wrote:
>> 
>> 
>> 
>> On Jul 7, 2018, at 12:36 PM, Tom Most <twm at freecog.net> 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/ <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 <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 <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 <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 artifacts with Travis or Circle somehow then we could just get rid of this.

-g
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20180708/c9bd54ee/attachment-0002.html>