On Sun, Jul 8, 2018, at 11:45 PM, Glyph wrote: >> On Jul 8, 2018, at 6:21 PM, Tom Most <t...@freecog.net> wrote: >> >> On Sun, Jul 8, 2018, at 12:06 PM, Glyph wrote: >>>> On Jul 8, 2018, at 7:44 AM, Adi Roiban <a...@roiban.ro> wrote: >>>> >>>> On Sat, 7 Jul 2018 at 22:45, Glyph <gl...@twistedmatrix.com> wrote:>>>>> >>>> On Jul 7, 2018, at 12:36 PM, Tom Most <t...@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
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
