[Twisted-Python] "Cleared to Land"?

Tue Jul 10 23:54:01 MDT 2018

On Sun, Jul 8, 2018, at 11:45 PM, Glyph wrote:
>> On Jul 8, 2018, at 6:21 PM, Tom Most <twm at freecog.net> wrote:
>> 
>> 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.> 
> I've just made you a trac admin, so you should be able to have
> at it :-).
Thanks, done! :-)

>>>>> 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.> 
> 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>>> 
>>> 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.> 
> Hmm.  Fair point there.  Label it is.
> 
>>>> 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]. :)> 
> "review requested" is pretty close to this state; I don't think *this*
> one needs a custom label? :)
Good point! Setting the "review" keyword in Trac should automatically
request a review from twisted/twisted-contributors. Adding a review
should remove the "review" keyword from Trac and add a comment to the
ticket which points at the review. Does that sound correct?
>> 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.> 
> This looks super interesting.  I would definitely like to have a
> commit queue, so that we don't have to sit around manually waiting on
> CI, but we don't have to reduce our CI coverage platforms (especially
> for alternate kernels, which it's really Twisted's job to cope with).> 
>>>> 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_sen-
>>> dmsg.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> Please go ahead and disable this test everywhere.  The test itself
> must be buggy, if it doesn't work in the Travis environment where
> presumably sendmsg actually works fine.  (Also, this is a 27-only test
> and if we wait long enough before fixing any underlying issue we can
> probably just delete it...)
Done! Per Adi's review I deleted it entirely.
https://github.com/twisted/twisted/pull/1038[5]
Thanks,
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
  5. https://github.com/twisted/twisted/pull/1038#pullrequestreview-135761507
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20180710/857e99d9/attachment-0002.html>