[Twisted-Python] Spurious commit/ticket links
Hi, In the last couple days I've noticed that there are a bunch of spurious changes being made to tickets in the issue tracker. These come from commit messages that reference a GitHub PR that happens to match a ticket number in trac. For example, https://twistedmatrix.com/trac/ticket/600#comment:11 I guess this doesn't really hurt anything ... except it's dumping a constant low level of garbage into the issue tracker and generating some annoying emails (that end up having nothing to do with what the subject suggests). Jean-Paul
This is, unfortunately, going to keep happening more frequently as the PR numbers get higher and the corresponding Trac tickets get less sparse. The way I'd like to address it is to change the format of our commit message to namespace Trac tickets differently; instead of just "#", using a URL, like "Fixes https://tm.tl/1234". I wouldn't even mind if we just had to use the Trac wiki syntax for this, i.e. "Fixes [ticket:1234]" as long as we could turn off the "#" syntax which Github also uses. However, this involves surgery within Trac's code, and for me personally, the work required to find the relevant regexes and modify them is worse than continuing to deal with the annoyance. However, I would very much appreciate it if someone else would take this on :-). -glyph
https://github.com/twisted-infra/twisted-trac-source https://github.com/twisted-infra/twisted-trac-plugins https://github.com/twisted-infra/braid/tree/master/services/trac
Is it actually possible to deploy modifications?
There's probably still an undocumented setup step or two that we've missed - but after following <https://github.com/twisted-infra/braid/blob/master/README.md> 'fab config.production trac.upgrade' ought to do the trick. Allegedly it's even possible to set up a test development environment as per <https://github.com/twisted-infra/braid/blob/master/README.md#vagrant> :-). I haven't made any major changes since all these docs were added so I'm just following them from the beginning for the first time myself now. but certainly the prod-deploy process has worked fine for me many times on various services.
I'll take a look, if so.
Please be vocal about any roadblocks you hit. The ops situation has improved a ton since the last time you looked, but (accordingly) it's also changed almost completely. Good luck - and hopefully you'll need a lot less of it than previously ;-). -glyph
On Thu, Dec 1, 2016 at 3:33 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
Here's a new plugin that has the new behavior, maybe: https://github.com/twisted-infra/twisted-trac-plugins/pull/4 thijs did a review so maybe it's all set to merge. There's also a config change: https://github.com/twisted-infra/braid/pull/226 I had intended to test it with the local development environment but it looks like it requires downloading gigs of data for vagrant boxes and to upgrade a super-old ubuntu image. Maybe someone else could do that step. If not, maybe I'll test it on the production machine in a couple days. :) Jean-Paul
I deployed this to production. I don't see a good way to test it without screwing around with twisted trunk ... so don't plan to. I'll keep an eye on real merges folks are working on and see if it's behaving as desired. On Thu, Dec 1, 2016 at 8:50 PM, Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
On Dec 2, 2016, at 4:49 PM, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:
I deployed this to production. I don't see a good way to test it without screwing around with twisted trunk ... so don't plan to. I'll keep an eye on real merges folks are working on and see if it's behaving as desired.
Thanks, JP! Would you mind updating https://twistedmatrix.com/trac/wiki/ReviewProcess#Revertingachange and https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechang... to reflect the new syntax that should be used on future merges? -glyph
On Fri, Dec 2, 2016 at 9:28 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
Updated. As far as the behavior, do https://github.com/twisted/twisted/pull/614 / http://twistedmatrix.com/trac/ticket/8934 look right? The trac ticket is fixed but with no comment. OTOH, I can't find the commit message for the PR merge so maybe that's as it should be?
On Sat, Dec 3, 2016 at 9:28 AM, Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
Updated again. Craig confirmed that it indeed wasn't working (at all). Also, the site's now running trac 1.2 (was 1.0.12). The fab target for re-deploying trac just picks the newest release at the moment, despite the *appearance* of being pinned to a particular version. I don't think I'm going to try to bite off fixing this. The problem, though, is multiple invocations of `pip -U` - a later instance of which is totally happy to upgrade the trac 1.0.12 install to 1.2. Jean-Paul
Now I can't create tickets at all. I hope this is related and not just general decay of our Trac instance :). The web UI tells me "Warning: The action "None" is not available" without actually creating the ticket. Nothing that I can see appears in the logs. Hopefully this warning message means something to someone? -glyph
Another problem commit: https://github.com/twisted/twisted/commit/6ec4521caae4edec97238ae00ff9bf81a4... This should have reopened https://twistedmatrix.com/trac/ticket/8937 but didn't. Have there been any examples of it working yet? -glyph
I did test it out, aaaaand: https://twistedmatrix.com/trac/ticket/8922#comment:5 It works! \o/ -glyph
On Thu, Dec 1, 2016 at 3:13 PM, Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
https://trac.edgewall.org/browser/trunk/tracopt/ticket/commit_updater.py#L14... looks like the relevant regexp. Removing the "#" branch would probably produce the desired behavior. However, also see https://trac.edgewall.org/wiki/CommitTicketUpdater#Configuration. I wonder if commit_ticket_update_envelope would also fix it? I don't think so because I think these are just "references" not "commands", but I don't know for sure. Jean-Paul
Jean-Paul
This is, unfortunately, going to keep happening more frequently as the PR numbers get higher and the corresponding Trac tickets get less sparse. The way I'd like to address it is to change the format of our commit message to namespace Trac tickets differently; instead of just "#", using a URL, like "Fixes https://tm.tl/1234". I wouldn't even mind if we just had to use the Trac wiki syntax for this, i.e. "Fixes [ticket:1234]" as long as we could turn off the "#" syntax which Github also uses. However, this involves surgery within Trac's code, and for me personally, the work required to find the relevant regexes and modify them is worse than continuing to deal with the annoyance. However, I would very much appreciate it if someone else would take this on :-). -glyph
https://github.com/twisted-infra/twisted-trac-source https://github.com/twisted-infra/twisted-trac-plugins https://github.com/twisted-infra/braid/tree/master/services/trac
Is it actually possible to deploy modifications?
There's probably still an undocumented setup step or two that we've missed - but after following <https://github.com/twisted-infra/braid/blob/master/README.md> 'fab config.production trac.upgrade' ought to do the trick. Allegedly it's even possible to set up a test development environment as per <https://github.com/twisted-infra/braid/blob/master/README.md#vagrant> :-). I haven't made any major changes since all these docs were added so I'm just following them from the beginning for the first time myself now. but certainly the prod-deploy process has worked fine for me many times on various services.
I'll take a look, if so.
Please be vocal about any roadblocks you hit. The ops situation has improved a ton since the last time you looked, but (accordingly) it's also changed almost completely. Good luck - and hopefully you'll need a lot less of it than previously ;-). -glyph
On Thu, Dec 1, 2016 at 3:33 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
Here's a new plugin that has the new behavior, maybe: https://github.com/twisted-infra/twisted-trac-plugins/pull/4 thijs did a review so maybe it's all set to merge. There's also a config change: https://github.com/twisted-infra/braid/pull/226 I had intended to test it with the local development environment but it looks like it requires downloading gigs of data for vagrant boxes and to upgrade a super-old ubuntu image. Maybe someone else could do that step. If not, maybe I'll test it on the production machine in a couple days. :) Jean-Paul
I deployed this to production. I don't see a good way to test it without screwing around with twisted trunk ... so don't plan to. I'll keep an eye on real merges folks are working on and see if it's behaving as desired. On Thu, Dec 1, 2016 at 8:50 PM, Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
On Dec 2, 2016, at 4:49 PM, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:
I deployed this to production. I don't see a good way to test it without screwing around with twisted trunk ... so don't plan to. I'll keep an eye on real merges folks are working on and see if it's behaving as desired.
Thanks, JP! Would you mind updating https://twistedmatrix.com/trac/wiki/ReviewProcess#Revertingachange and https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechang... to reflect the new syntax that should be used on future merges? -glyph
On Fri, Dec 2, 2016 at 9:28 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
Updated. As far as the behavior, do https://github.com/twisted/twisted/pull/614 / http://twistedmatrix.com/trac/ticket/8934 look right? The trac ticket is fixed but with no comment. OTOH, I can't find the commit message for the PR merge so maybe that's as it should be?
On Sat, Dec 3, 2016 at 9:28 AM, Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
Updated again. Craig confirmed that it indeed wasn't working (at all). Also, the site's now running trac 1.2 (was 1.0.12). The fab target for re-deploying trac just picks the newest release at the moment, despite the *appearance* of being pinned to a particular version. I don't think I'm going to try to bite off fixing this. The problem, though, is multiple invocations of `pip -U` - a later instance of which is totally happy to upgrade the trac 1.0.12 install to 1.2. Jean-Paul
Now I can't create tickets at all. I hope this is related and not just general decay of our Trac instance :). The web UI tells me "Warning: The action "None" is not available" without actually creating the ticket. Nothing that I can see appears in the logs. Hopefully this warning message means something to someone? -glyph
Another problem commit: https://github.com/twisted/twisted/commit/6ec4521caae4edec97238ae00ff9bf81a4... This should have reopened https://twistedmatrix.com/trac/ticket/8937 but didn't. Have there been any examples of it working yet? -glyph
I did test it out, aaaaand: https://twistedmatrix.com/trac/ticket/8922#comment:5 It works! \o/ -glyph
On Thu, Dec 1, 2016 at 3:13 PM, Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
https://trac.edgewall.org/browser/trunk/tracopt/ticket/commit_updater.py#L14... looks like the relevant regexp. Removing the "#" branch would probably produce the desired behavior. However, also see https://trac.edgewall.org/wiki/CommitTicketUpdater#Configuration. I wonder if commit_ticket_update_envelope would also fix it? I don't think so because I think these are just "references" not "commands", but I don't know for sure. Jean-Paul
Jean-Paul
participants (2)
-
Glyph Lefkowitz -
Jean-Paul Calderone