> 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/
> <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
_______________________________________________
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
