Discussion:
[Twisted-Python] Spurious commit/ticket links
Jean-Paul Calderone
2016-12-01 18:51:42 UTC
Permalink
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
Glyph Lefkowitz
2016-12-01 19:14:10 UTC
Permalink
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 <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).
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
Jean-Paul Calderone
2016-12-01 20:13:11 UTC
Permalink
On Dec 1, 2016, at 10:51 AM, Jean-Paul Calderone <
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).
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 :-).
Where's the source for Twisted's trac deployment? Is it actually possible
to deploy modifications? I'll take a look, if so.

Jean-Paul
Glyph Lefkowitz
2016-12-01 20:33:43 UTC
Permalink
Post by Glyph Lefkowitz
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 <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).
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 <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 :-).
Where's the source for Twisted's trac deployment?
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
Post by Glyph Lefkowitz
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.
Post by Glyph Lefkowitz
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
Jean-Paul Calderone
2016-12-02 01:50:54 UTC
Permalink
Post by Glyph Lefkowitz
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 ;-).
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
Jean-Paul Calderone
2016-12-03 00:49:38 UTC
Permalink
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 <
Post by Jean-Paul Calderone
Post by Glyph Lefkowitz
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 ;-).
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
Glyph Lefkowitz
2016-12-03 02:28:28 UTC
Permalink
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:Howtomergethechangetotrunk to reflect the new syntax that should be used on future merges?

-glyph
Jean-Paul Calderone
2016-12-03 14:28:42 UTC
Permalink
On Dec 2, 2016, at 4:49 PM, Jean-Paul Calderone <
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:Howtomergethechangetotrunk to reflect the new syntax that should
be used on future merges?
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?
-glyph
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
Jean-Paul Calderone
2016-12-05 22:45:12 UTC
Permalink
On Sat, Dec 3, 2016 at 9:28 AM, Jean-Paul Calderone <
On Dec 2, 2016, at 4:49 PM, Jean-Paul Calderone <
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:Howtomergethechangetotrunk to reflect the
new syntax that should be used on future merges?
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?
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
-glyph
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
Glyph Lefkowitz
2016-12-06 06:08:24 UTC
Permalink
Post by Glyph Lefkowitz
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 <https://twistedmatrix.com/trac/wiki/ReviewProcess#Revertingachange> and https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechangetotrunk <https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechangetotrunk> to reflect the new syntax that should be used on future merges?
Updated. As far as the behavior, do https://github.com/twisted/twisted/pull/614 <https://github.com/twisted/twisted/pull/614> / http://twistedmatrix.com/trac/ticket/8934 <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?
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.
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
Jean-Paul Calderone
2016-12-06 13:04:11 UTC
Permalink
Post by Glyph Lefkowitz
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?
Not to me, unfortunately. :(
Post by Glyph Lefkowitz
-glyph
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
Glyph Lefkowitz
2016-12-06 17:55:35 UTC
Permalink
Post by Glyph Lefkowitz
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?
Not to me, unfortunately. :(
Luckily it meant something to Amber!

-glyph
Glyph Lefkowitz
2016-12-09 01:49:05 UTC
Permalink
Post by Jean-Paul Calderone
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.
Another problem commit:

https://github.com/twisted/twisted/commit/6ec4521caae4edec97238ae00ff9bf81a4701a73

This should have reopened https://twistedmatrix.com/trac/ticket/8937 but didn't.

Have there been any examples of it working yet?

-glyph
Jean-Paul Calderone
2016-12-09 03:03:49 UTC
Permalink
Post by Jean-Paul Calderone
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.
https://github.com/twisted/twisted/commit/6ec4521caae4edec97238ae00ff9bf
81a4701a73
This should have reopened https://twistedmatrix.com/trac/ticket/8937 but didn't.
Have there been any examples of it working yet?
(Followed-up on IRC in Twisted admin channe; glyph can summarize if he
Post by Jean-Paul Calderone
wants.
-glyph
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
Glyph Lefkowitz
2016-12-09 06:54:30 UTC
Permalink
Post by Jean-Paul Calderone
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.
https://github.com/twisted/twisted/commit/6ec4521caae4edec97238ae00ff9bf81a4701a73 <https://github.com/twisted/twisted/commit/6ec4521caae4edec97238ae00ff9bf81a4701a73>
This should have reopened https://twistedmatrix.com/trac/ticket/8937 <https://twistedmatrix.com/trac/ticket/8937> but didn't.
Have there been any examples of it working yet?
(Followed-up on IRC in Twisted admin channe; glyph can summarize if he wants.
The part that I remember was just fixing trac after it broke - but hopefully what broke it was an update that fixes the regex :). Hopefully I'll close another ticket tonight and test it out...

-g
Glyph Lefkowitz
2016-12-09 10:04:22 UTC
Permalink
Post by Glyph Lefkowitz
Post by Jean-Paul Calderone
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.
https://github.com/twisted/twisted/commit/6ec4521caae4edec97238ae00ff9bf81a4701a73 <https://github.com/twisted/twisted/commit/6ec4521caae4edec97238ae00ff9bf81a4701a73>
This should have reopened https://twistedmatrix.com/trac/ticket/8937 <https://twistedmatrix.com/trac/ticket/8937> but didn't.
Have there been any examples of it working yet?
(Followed-up on IRC in Twisted admin channe; glyph can summarize if he wants.
The part that I remember was just fixing trac after it broke - but hopefully what broke it was an update that fixes the regex :). Hopefully I'll close another ticket tonight and test it out...
I did test it out, aaaaand:

https://twistedmatrix.com/trac/ticket/8922#comment:5

It works! \o/

-glyph

Jean-Paul Calderone
2016-12-01 20:34:36 UTC
Permalink
On Thu, Dec 1, 2016 at 3:13 PM, Jean-Paul Calderone <
Post by Jean-Paul Calderone
On Dec 1, 2016, at 10:51 AM, Jean-Paul Calderone <
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).
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 :-).
Where's the source for Twisted's trac deployment? Is it actually possible
to deploy modifications? I'll take a look, if so.
https://trac.edgewall.org/browser/trunk/tracopt/ticket/commit_updater.py#L140
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
Post by Jean-Paul Calderone
Jean-Paul
Loading...