[Twisted-Python] "Cleared to Land"?
I have a minor PR that was reviewed and labeled "cleared to land" a while back, but three months have passed since then and it's still unmerged. Is there something else I need to do to move this along, or did it just fall through the cracks?
Wim.
On Jul 6, 2018, at 4:11 PM, Wim Lewis wiml@omnigroup.com wrote:
I have a minor PR that was reviewed and labeled "cleared to land" a while back, but three months have passed since then and it's still unmerged. Is there something else I need to do to move this along, or did it just fall through the cracks?
Wim.
Hi Wim,
Can you include a link to to the issue in question? Probably someone (me?) intended to indicate that it was in the appropriate workflow state for landing, but was waiting for continuous integration to complete, and wandered off and forgot about it. If you can pester a contributor here, or on IRC, someone should be able to land it for you.
Speaking of which, it seems like you've been a fairly stalwart and patient external contributor. Would you like to join the project as a member and merge your own approved stuff to trunk? Mostly, the attendant responsibilities are documented here:
https://twistedmatrix.com/trac/wiki/ReviewProcess https://twistedmatrix.com/trac/wiki/ReviewProcess
(This used to be the authoritative list - https://twistedmatrix.com/trac/wiki/CommitterCheckList https://twistedmatrix.com/trac/wiki/CommitterCheckList - and I've just updated it to remove some extraneous stuff, but it seems that pervasive cultural understanding of github's workflow have obviated the need for repetitive stern cautions that one should not commit directly to trunk without code review and CI...)
-g
On Sat, Jul 7, 2018, at 12:08 AM, Glyph wrote:
On Jul 6, 2018, at 4:11 PM, Wim Lewis wiml@omnigroup.com wrote:
I have a minor PR that was reviewed and labeled "cleared to land" a while back, but three months have passed since then and it's still unmerged. Is there something else I need to do to move this along, or>> did it just fall through the cracks?
Wim.
Hi Wim,
Can you include a link to to the issue in question? Probably someone (me?) intended to indicate that it was in the appropriate workflow state for landing, but was waiting for continuous integration to complete, and wandered off and forgot about it. If you can pester a contributor here, or on IRC, someone should be able to land it for you.> Speaking of which, it seems like you've been a fairly stalwart and patient external contributor. Would you like to join the project as a member and merge your own approved stuff to trunk? Mostly, the attendant responsibilities are documented here:> https://twistedmatrix.com/trac/wiki/ReviewProcess
(This used to be the authoritative list - https://twistedmatrix.com/trac/wiki/CommitterCheckList - and I've just updated it to remove some extraneous stuff, but it seems that pervasive cultural understanding of github's workflow have obviated the need for repetitive stern cautions that one should not commit directly to trunk without code review and CI...)> -g
Hi all,
Perhaps it was https://github.com/twisted/twisted/pull/991? It was tagged "cleared to land", but had build failures unrelated to the changes. I merged forward, pushed to branch, kicked a failing build, and got the green checkmark. It has been merged. 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. Also, yes, welcome Wim!
---Tom
On Jul 7, 2018, at 12:36 PM, Tom Most twm@freecog.net wrote:
On Sat, Jul 7, 2018, at 12:08 AM, Glyph wrote:
On Jul 6, 2018, at 4:11 PM, Wim Lewis <wiml@omnigroup.com mailto:wiml@omnigroup.com> wrote:
I have a minor PR that was reviewed and labeled "cleared to land" a while back, but three months have passed since then and it's still unmerged. Is there something else I need to do to move this along, or did it just fall through the cracks?
Wim.
Hi Wim,
Can you include a link to to the issue in question? Probably someone (me?) intended to indicate that it was in the appropriate workflow state for landing, but was waiting for continuous integration to complete, and wandered off and forgot about it. If you can pester a contributor here, or on IRC, someone should be able to land it for you.
Speaking of which, it seems like you've been a fairly stalwart and patient external contributor. Would you like to join the project as a member and merge your own approved stuff to trunk? Mostly, the attendant responsibilities are documented here:
https://twistedmatrix.com/trac/wiki/ReviewProcess https://twistedmatrix.com/trac/wiki/ReviewProcess
(This used to be the authoritative list - https://twistedmatrix.com/trac/wiki/CommitterCheckList https://twistedmatrix.com/trac/wiki/CommitterCheckList - and I've just updated it to remove some extraneous stuff, but it seems that pervasive cultural understanding of github's workflow have obviated the need for repetitive stern cautions that one should not commit directly to trunk without code review and CI...)
-g
Hi all,
Perhaps it was https://github.com/twisted/twisted/pull/991? https://github.com/twisted/twisted/pull/991?
Hah, it was Mark, not me! Score!
It was tagged "cleared to land", but had build failures unrelated to the changes. I merged forward, pushed to branch, kicked a failing build, and got the green checkmark. It has been merged.
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:Howtomergethechang... 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/ https://twistedmatrix.com/highscores/.
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/ 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 https://github.com/twisted/twisted/pull/1010 has suffered the same fate? Any other committers have a minute to land that one? :)
-g
On Sat, 7 Jul 2018 at 22:45, Glyph glyph@twistedmatrix.com wrote:
On Jul 7, 2018, at 12:36 PM, Tom Most twm@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:Howtomergethechang...
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/
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
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.
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?
-----
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
On Jul 8, 2018, at 7:44 AM, Adi Roiban adi@roiban.ro wrote:
On Sat, 7 Jul 2018 at 22:45, Glyph glyph@twistedmatrix.com wrote:
On Jul 7, 2018, at 12:36 PM, Tom Most twm@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:Howtomergethechang...
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
On Sun, Jul 8, 2018, at 12:06 PM, Glyph wrote:
On Jul 8, 2018, at 7:44 AM, Adi Roiban adi@roiban.ro wrote:
On Sat, 7 Jul 2018 at 22:45, Glyph glyph@twistedmatrix.com wrote:
On Jul 7, 2018, at 12:36 PM, Tom Most twm@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:Howtomergethechang... 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/.%3E%3E
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%3E
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%3E%3E 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
On Sun, Jul 8, 2018, at 6:21 PM, Tom Most wrote:
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.
Please disregard this. I tried it on #1037[2] but branch protection still applies: $ git push origin trunk Counting objects: 6, done. Delta compression using up to 4 threads. Compressing objects: 100% (5/5), done. Writing objects: 100% (6/6), 610 bytes | 0 bytes/s, done. Total 6 (delta 4), reused 0 (delta 0) remote: Resolving deltas: 100% (4/4), completed with 4 local objects. remote: error: GH006: Protected branch update failed for refs/heads/trunk.remote: error: 3 of 3 required status checks are expected. To git@github.com:twisted/twisted.git ! [remote rejected] trunk -> trunk (protected branch hook declined) error: failed to push some refs to 'git@github.com:twisted/twisted.git' ---Tom
Links:
1. https://github.com/twisted/twisted/pull/1037 2. https://github.com/twisted/twisted/pull/1037
On Jul 8, 2018, at 6:21 PM, Tom Most twm@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@roiban.ro mailto:adi@roiban.ro> wrote:
On Sat, 7 Jul 2018 at 22:45, Glyph <glyph@twistedmatrix.com mailto:glyph@twistedmatrix.com> wrote:
On Jul 7, 2018, at 12:36 PM, Tom Most <twm@freecog.net mailto:twm@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:Howtomergethechang... 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/ 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.
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 :-).
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/ 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 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 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 https://github.com/twisted/twisted/pull/1037. 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 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 https://github.com/twisted/twisted/pull/1021 out of sync https://github.com/twisted/twisted/pull/1037. :)
"review requested" is pretty close to this state; I don't think this one needs a custom label? :)
Have you seen the Bors approach https://github.com/graydon/bors? 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 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 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...)
-g
On Sun, Jul 8, 2018, at 11:45 PM, Glyph wrote:
On Jul 8, 2018, at 6:21 PM, Tom Most twm@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@roiban.ro wrote:
On Sat, 7 Jul 2018 at 22:45, Glyph glyph@twistedmatrix.com wrote:>>>>> On Jul 7, 2018, at 12:36 PM, Tom Most twm@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:Howtomergethechang... 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/.%3E%3E%3E%3E
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%3E%...
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%3E%3E%3E%3E 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%3E 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%5B5] 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
On Sat, 7 Jul 2018 00:08:51 -0700, Glyph glyph@twistedmatrix.com wrote:
Can you include a link to to the issue in question?
Sorry, it was trac 9413 / github 991, which Tom Most helpfully merged after I wrote. (Thanks Tom!)
Speaking of which, it seems like you've been a fairly stalwart and patient external contributor. Would you like to join the project as a member and merge your own approved stuff to trunk? Mostly, the attendant responsibilities are documented here:
I'd be honored. At the very least I could re-trigger CI builds when they fail... :)
(I should say that I've been contributing more to Twisted lately because I'm working on a project using it, and I'll probably have much less time for Twisted development once that project is put to bed. Until the next project, I suppose.)
Wim.
On Jul 16, 2018, at 9:33 PM, Wim Lewis wiml@omnigroup.com wrote:
On Sat, 7 Jul 2018 00:08:51 -0700, Glyph glyph@twistedmatrix.com wrote:
Can you include a link to to the issue in question?
Sorry, it was trac 9413 / github 991, which Tom Most helpfully merged after I wrote. (Thanks Tom!)
Speaking of which, it seems like you've been a fairly stalwart and patient external contributor. Would you like to join the project as a member and merge your own approved stuff to trunk? Mostly, the attendant responsibilities are documented here:
I'd be honored. At the very least I could re-trigger CI builds when they fail... :)
Congratulations. You should have an email which, when you click the link on it, will make it official.
Use your powers only for good, never for evil.
(I should say that I've been contributing more to Twisted lately because I'm working on a project using it, and I'll probably have much less time for Twisted development once that project is put to bed. Until the next project, I suppose.)
We all wax and wane according to the season :-).
-g
participants (4)
-
Adi Roiban -
Glyph -
Tom Most -
Wim Lewis