[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 <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/
> 
> 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 "<comma_separated_names>" in the merge
commit message example to "<comma_separated_github_usernames>"? 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 you seen the Bors approach[4]? Roughly, Bors is a bot which merges
a PR to the mainline on a temporary branch, runs the tests, and then fast-
forwards the mainline if they pass. If we took that approach we'd not
have to manually merge forward all the time.
>> 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.
This failure has been wasting a lot of time over the past two months. I
think that we should disable this test on Travis until we have a proper
fix. It still runs on Buildbot, no? Buggy tests are worse than no
tests, as they let one portion of the project prevent forward progress
on all others.
The rhel7-py27-coverage builder seems to be having issues downloading
packages from PyPI, too. Fortunately it fails fast and the queue is
usually short so it's a quick to kick:
https://buildbot.twistedmatrix.com/builders/rhel7-py2.7-coverage
---Tom

Links:

  1. https://github.com/twisted/twisted/pull/1037
  2. https://github.com/twisted/twisted/pull/1021
  3. https://github.com/twisted/twisted/pull/1037
  4. https://github.com/graydon/bors
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20180708/d31737de/attachment-0002.html>