Trac workflow and needs_review

[Erik Bray]

Hi all,

The current ticket workflow in Trac only allows creating new tickets
with a status 'new'. So when posting a branch with proposed changes,
for example it is necessary to create the ticket, and then immediately
update it to needs_review.

This is fine for tickets that don't immediately have code. But for
those that do it would be nice to go straight to needs_review. Any
objection to changing the workflow to allow this? Perhaps if it
helps, we can restrict it to require certain fields to be set (Author,
Branch, etc.) to allow setting needs_review at all.

Erik

[Vincent Delecroix]

+1
I am all for it (with restriction for switching to needs review).

[Erik Bray]

On Tue, Jun 28, 2016 at 4:06 PM, Vincent Delecroix

While we're tinkering with the workflow, I think we need to change the
workflow associated with testing changes.

Currently the release manager *closes* a ticket and marks it as
"fixed" when merging the change for testing. This really makes no
sense--if the tests fail the issue is not "fixed". After
"positive_review" can we add a "in testing" status or the like (from
which the next stages can be either back to "needs_work" or
"resolved".

[leif]

Erik Bray wrote:
> While we're tinkering with the workflow, I think we need to change the
> workflow associated with testing changes.
>
> Currently the release manager *closes* a ticket and marks it as

s/currently the release manager/the current release manager/ ;-)

> "fixed" when merging the change for testing. This really makes no
> sense--if the tests fail the issue is not "fixed". After
> "positive_review" can we add a "in testing" status or the like (from
> which the next stages can be either back to "needs_work" or
> "resolved".

We had at least one lengthy discussion regarding that in the past (last
time probably a year ago? -- too lazy to search the thread[s] now, but
they're certainly worth reading [again]).

Nothing happened since then though, if I'm not mistaken.

But note that the buildbots do test tickets having positive review, some
also those with "needs review", so with relatively frequent "beta
releases", it's not all that bad.


-leif

[leif]

leif wrote:
> Erik Bray wrote:
>> While we're tinkering with the workflow, I think we need to change the
>> workflow associated with testing changes.
>>
>> Currently the release manager *closes* a ticket and marks it as
>
> s/currently the release manager/the current release manager/ ;-)
>
>> "fixed" when merging the change for testing. This really makes no
>> sense--if the tests fail the issue is not "fixed". After
>> "positive_review" can we add a "in testing" status or the like (from
>> which the next stages can be either back to "needs_work" or
>> "resolved".
>
> We had at least one lengthy discussion regarding that in the past (last
> time probably a year ago? -- too lazy to search the thread[s] now, but
> they're certainly worth reading [again]).

See (for example) "Race condition when closing tickets" [1] and its
"follow-up" "Pushing to tickets after setting it to positive_review is
incompatible with the current workflow" [2] from April/May last year.


-leif

[1] http://thread.gmane.org/gmane.comp.mathematics.sage.devel/78952

[2] http://thread.gmane.org/gmane.comp.mathematics.sage.devel/79018

[Volker Braun]

On Tuesday, June 28, 2016 at 6:23:18 PM UTC+2, Erik Bray wrote:

Currently the release manager *closes* a ticket and marks it as
"fixed" when merging the change for testing.

No. Tickets are closed *after* tests succeed. 

The only race condition is if somebody else reopens positively reviewed tickets whose tests pass. Arguably that shouldn't be done, since that ticket is complete. There is always something else to do, just open a new ticket. Instead of reopening an old ticket to pile on more commits. 

[Erik Bray]

On Tue, Jun 28, 2016 at 7:14 PM, leif <not.r...@online.de> wrote:
> Erik Bray wrote:
>> While we're tinkering with the workflow, I think we need to change the
>> workflow associated with testing changes.
>>
>> Currently the release manager *closes* a ticket and marks it as
>
> s/currently the release manager/the current release manager/ ;-)

Yes, but I didn't want to imply "fault" here or anything like that.
Volker does what's best for Volker and that's completely fine.

The problem is that there aren't any documented procedures (that I
know of!) for how testing of issues, merging, and releases should be
handled. So the correct procedure *is* whatever the current release
manager does. That, I do have a problem with, though it's nobody's
fault.

>> "fixed" when merging the change for testing. This really makes no
>> sense--if the tests fail the issue is not "fixed". After
>> "positive_review" can we add a "in testing" status or the like (from
>> which the next stages can be either back to "needs_work" or
>> "resolved".
>
> We had at least one lengthy discussion regarding that in the past (last
> time probably a year ago? -- too lazy to search the thread[s] now, but
> they're certainly worth reading [again]).
>
> Nothing happened since then though, if I'm not mistaken.
>
> But note that the buildbots do test tickets having positive review, some
> also those with "needs review", so with relatively frequent "beta
> releases", it's not all that bad.

I'm really confused about what the buildbots do or don't do. Or what
the relationship is between patchbot and buildbot (other than the
latter implying that someone is managing a bunch of buildbot instances
somewhere?)

[Erik Bray]

On Tue, Jun 28, 2016 at 10:20 PM, Volker Braun <vbrau...@gmail.com> wrote:
> On Tuesday, June 28, 2016 at 6:23:18 PM UTC+2, Erik Bray wrote:
>>
>> Currently the release manager *closes* a ticket and marks it as
>> "fixed" when merging the change for testing.
>
>
> No. Tickets are closed *after* tests succeed.

I'm just going by what Jeroen told me here:
https://trac.sagemath.org/ticket/20423#comment:12

I feel like I've also seen a ticket of mine closed and reopened again
for exactly this reason, unless that's changed.

> The only race condition is if somebody else reopens positively reviewed
> tickets whose tests pass. Arguably that shouldn't be done, since that ticket
> is complete. There is always something else to do, just open a new ticket.
> Instead of reopening an old ticket to pile on more commits.

Agreed here. If it's closed+merged it should remain closed, unless it
was just closed by accident.

[leif]

Erik Bray wrote:
> On Tue, Jun 28, 2016 at 7:14 PM, leif <not.r...@online.de> wrote:
>> Erik Bray wrote:
>>> While we're tinkering with the workflow, I think we need to change the
>>> workflow associated with testing changes.
>>>
>>> Currently the release manager *closes* a ticket and marks it as
>>
>> s/currently the release manager/the current release manager/ ;-)
>
> Yes, but I didn't want to imply "fault" here or anything like that.
> Volker does what's best for Volker and that's completely fine.
>
> The problem is that there aren't any documented procedures (that I
> know of!) for how testing of issues, merging, and releases should be
> handled. So the correct procedure *is* whatever the current release
> manager does. That, I do have a problem with, though it's nobody's
> fault.

Regarding the release process, there are some (fairly outdated) notes in
the Sage wiki, but a lot is simply defined by the actual release
manager(s) (and more or less tradition). In the past, the release
managers (usually teams) changed more often, also because nobody
volunteered (or had the time to do it) for a longer period.

>>> "fixed" when merging the change for testing. This really makes no
>>> sense--if the tests fail the issue is not "fixed". After
>>> "positive_review" can we add a "in testing" status or the like (from
>>> which the next stages can be either back to "needs_work" or
>>> "resolved".
>>
>> We had at least one lengthy discussion regarding that in the past (last
>> time probably a year ago? -- too lazy to search the thread[s] now, but
>> they're certainly worth reading [again]).
>>
>> Nothing happened since then though, if I'm not mistaken.
>>
>> But note that the buildbots do test tickets having positive review, some
>> also those with "needs review", so with relatively frequent "beta
>> releases", it's not all that bad.
>
> I'm really confused about what the buildbots do or don't do. Or what
> the relationship is between patchbot and buildbot (other than the
> latter implying that someone is managing a bunch of buildbot instances
> somewhere?)

My mistake, tickets are automatically tested by our *patchbots*.

The buildbots don't do anything by themselves (unless the release
manager runs some cron job, say). They are used to test potential
(beta/rc/final) releases before releasing, and to build binary
distributions of Sage.


All of the above testing of course makes up only a fraction of the
review process though (hopefully, I mean).

People should really read the code, diffs, comments by other reviewers
on a ticket, and experiment interactively, otherwise we could automate
setting tickets to "positive review" (and merging them) as well... B)

IMHO there have been (and will be) /some/ tickets with good reasons to
revert them from positive review back to "needs work" or "needs review"
(despite their patches applying and their tests -- if any -- passing),
i.e., where merging them as is and opening follow-ups (which in fact
rarely happens, often not because it wasn't necessary) instead didn't
make sense, hence 'in_testing' or whatever seems useful. But the main
problem usually is time, namely the time for others to take a look and
to eventually intervene between someone setting a ticket to positive
review and the ticket getting "finally" merged (into the release
managers' trunk).

'in_testing' or 'merge_candidate' could attract (or alert) more people
to look at, but the impact of a change (beyond what files get touched --
we don't even have a tool to notify people based on that) also matters.
Everyone is allowed to (attempt to) modify any part of Sage, while we
don't all live in the same timezone, and most of us don't work full-time
on Sage.


-leif

[William Stein]

On Thu, Jun 30, 2016 at 8:35 AM, Erik Bray <erik....@gmail.com> wrote:
> On Tue, Jun 28, 2016 at 7:14 PM, leif <not.r...@online.de> wrote:
>> Erik Bray wrote:
>>> While we're tinkering with the workflow, I think we need to change the
>>> workflow associated with testing changes.
>>>
>>> Currently the release manager *closes* a ticket and marks it as
>>
>> s/currently the release manager/the current release manager/ ;-)
>
> Yes, but I didn't want to imply "fault" here or anything like that.
> Volker does what's best for Volker and that's completely fine.
>
> The problem is that there aren't any documented procedures (that I
> know of!) for how testing of issues, merging, and releases should be
> handled. So the correct procedure *is* whatever the current release
> manager does. That, I do have a problem with, though it's nobody's
> fault.

Erik,

You are just begging to take a *turn* as release manager...

Incidentally, when I stopped being release manager, after several
years when I suddenly realized that I wasn't the only person in the
world who could do it, we had rotating release managers for a while...
which resulted in documenting workflows at the time. I don't
want to speak for Volker [*], but I imagine he might enjoy taking a
little break?

- William

[*] who is ridiculously good at release managing Sage, to put it
mildly. Doing Sage release management is very, very difficult.


--
William (http://wstein.org)

[Erik Bray]

On Thu, Jun 30, 2016 at 11:58 PM, William Stein <wst...@gmail.com> wrote:
> On Thu, Jun 30, 2016 at 8:35 AM, Erik Bray <erik....@gmail.com> wrote:
>> On Tue, Jun 28, 2016 at 7:14 PM, leif <not.r...@online.de> wrote:
>>> Erik Bray wrote:
>>>> While we're tinkering with the workflow, I think we need to change the
>>>> workflow associated with testing changes.
>>>>
>>>> Currently the release manager *closes* a ticket and marks it as
>>>
>>> s/currently the release manager/the current release manager/ ;-)
>>
>> Yes, but I didn't want to imply "fault" here or anything like that.
>> Volker does what's best for Volker and that's completely fine.
>>
>> The problem is that there aren't any documented procedures (that I
>> know of!) for how testing of issues, merging, and releases should be
>> handled. So the correct procedure *is* whatever the current release
>> manager does. That, I do have a problem with, though it's nobody's
>> fault.
>
> Erik,
>
> You are just begging to take a *turn* as release manager...

Well no, definitely not. Especially not in the way the role is
currently defined. That's a large part of the point--the current
definition of the role puts far too much work on one person, and
there's no enough documentation to make it easier to share the burden.

One thing that will help, which has already been discussed up-thread,
is having Trac help take care of the little nitty-gritty checks on
tickets. I want to make life easier for whoever is the release
manager, who shouldn't have to have much to do except when it comes
time to make actual releases.

> Incidentally, when I stopped being release manager, after several
> years when I suddenly realized that I wasn't the only person in the
> world who could do it, we had rotating release managers for a while...
> which resulted in documenting workflows at the time. I don't
> want to speak for Volker [*], but I imagine he might enjoy taking a
> little break?
>
> - William
>
> [*] who is ridiculously good at release managing Sage, to put it
> mildly. Doing Sage release management is very, very difficult.

Yep. Absolutely none of this is to criticize Volker. I think we
should make his life easier.

[Jori Mäntysalo]

On Fri, 1 Jul 2016, Erik Bray wrote:

> One thing that will help, which has already been discussed up-thread,
> is having Trac help take care of the little nitty-gritty checks on
> tickets.

True. It feels extremely stupid to get a message "You forgot reviewer's
name.", as this is exactly what computers should do instead of humans.

--
Jori Mäntysalo

[leif]

You cannot automatically add these names; all we could do is create some
plug-in such that when a ticket is set to "positive review" (and perhaps
also other states), the contents of the field gets checked. (Same for
the author(s) when the ticket gets into "needs review".)

If it's empty, we could offer a multiple-choice list of names of the
participants of the ticket *mapped to their real names*, but our
currently rather informal list of the latter [1] isn't complete. (A
separate file / database would be good anyway, and trac already has
something like that.)

We may also change these fields to contain trac account names, but
that's a matter of taste (and tradition), too.


I think Jeroen *did* automate the posts regarding empty author and/or
reviewer fields when he was release manager. :-)


-leif

[1] https://trac.sagemath.org/wiki#AccountNamesMappedtoRealNames

[Jori Mäntysalo]

On Fri, 1 Jul 2016, leif wrote:

>> True. It feels extremely stupid to get a message "You forgot reviewer's
>> name.", as this is exactly what computers should do instead of humans.
>
> You cannot automatically add these names; all we could do is create some
> plug-in such that when a ticket is set to "positive review" (and perhaps
> also other states), the contents of the field gets checked.

That would be enought.

--
Jori Mäntysalo

[kcrisman]

I think Jeroen *did* automate the posts regarding empty author and/or
reviewer fields when he was release manager. :-)

I'm pretty sure that Volker automates this too...

[Volker Braun]

No I haven't. Really it should be a trac plugin that prevents you from setting a ticket to positive review without filling out Author and Reviewer fields.

A somewhat orthogonal issue is whether to record plain text names or user ids. The advantage of plain text is that anyone can be listed (), without requiring an account. The advantage of user ids would be that people wouldn't misspell their names all the time. Still, I'd say that this is a minor concern right now.

[Erik Bray]

On Fri, Jul 1, 2016 at 8:11 PM, leif <not.r...@online.de> wrote:
> Jori Mäntysalo wrote:
>> On Fri, 1 Jul 2016, Erik Bray wrote:
>>
>>> One thing that will help, which has already been discussed up-thread,
>>> is having Trac help take care of the little nitty-gritty checks on
>>> tickets.
>>
>> True. It feels extremely stupid to get a message "You forgot reviewer's
>> name.", as this is exactly what computers should do instead of humans.
>
> You cannot automatically add these names; all we could do is create some
> plug-in such that when a ticket is set to "positive review" (and perhaps
> also other states), the contents of the field gets checked. (Same for
> the author(s) when the ticket gets into "needs review".)
>
> If it's empty, we could offer a multiple-choice list of names of the
> participants of the ticket *mapped to their real names*, but our
> currently rather informal list of the latter [1] isn't complete. (A
> separate file / database would be good anyway, and trac already has
> something like that.)

This is straightforward to add. I think it should be required when
moving a ticket to the "positive_review" status that a Reviewer be
specified. By default this can be a drop-down list of participants in
the ticket, taking their full names from Trac or, if not available,
their trac usernames (but in that case we should encourage people to
fill in their full names in Trac!) That said the person moving the
ticket to "positive_review" can also manually fill in that name in the
normal ticket box (e.g. if the code was reviewed by someone outside of
Trac).

Similarly for moving the ticket to needs_review, by default it can
fill in the current user's name as the Author, but of course that
doesn't prevent setting it to something else.

Ultimately it's good to have a human being look all this over before
merging, but if the guidelines are well spelled out somewhere that
does not have to be the job of one solitary person.

> We may also change these fields to contain trac account names, but
> that's a matter of taste (and tradition), too.

Yes, either way.

[leif]

Erik Bray wrote:
> On Fri, Jul 1, 2016 at 8:11 PM, leif <not.r...@online.de> wrote:
>> Jori Mäntysalo wrote:
>>> On Fri, 1 Jul 2016, Erik Bray wrote:
>>>
>>>> One thing that will help, which has already been discussed up-thread,
>>>> is having Trac help take care of the little nitty-gritty checks on
>>>> tickets.
>>>
>>> True. It feels extremely stupid to get a message "You forgot reviewer's
>>> name.", as this is exactly what computers should do instead of humans.
>>
>> You cannot automatically add these names; all we could do is create some
>> plug-in such that when a ticket is set to "positive review" (and perhaps
>> also other states), the contents of the field gets checked. (Same for
>> the author(s) when the ticket gets into "needs review".)
>>
>> If it's empty, we could offer a multiple-choice list of names of the
>> participants of the ticket *mapped to their real names*, but our
>> currently rather informal list of the latter [1] isn't complete. (A
>> separate file / database would be good anyway, and trac already has
>> something like that.)
>
> This is straightforward to add.

Well, go ahead... :P

> I think it should be required when
> moving a ticket to the "positive_review" status that a Reviewer be
> specified. By default this can be a drop-down list of participants in
> the ticket, taking their full names from Trac or, if not available,
> their trac usernames (but in that case we should encourage people to
> fill in their full names in Trac!) That said the person moving the
> ticket to "positive_review" can also manually fill in that name in the
> normal ticket box (e.g. if the code was reviewed by someone outside of
> Trac).
>
> Similarly for moving the ticket to needs_review, by default it can
> fill in the current user's name as the Author, but of course that
> doesn't prevent setting it to something else.

Yep, that's what I meant. (Although tickets which get closed as
"duplicate/invalid/won't fix" traditionally have no author but only
reviewer(s).)

Just note that we in fact have two "databases", a public and a private
one, namely the list on Sage's trac wiki page, and the records in trac's
account database. In the long run, we could probably "merge" them by
generating the former from the latter (and to achieve this, add some
fields to the preferences of each account, such as "public name" and
"institution", "location", web link, whatever).

> Ultimately it's good to have a human being look all this over before
> merging, but if the guidelines are well spelled out somewhere that
> does not have to be the job of one solitary person.

I think every release manager used his own scripts to perform
consistency checks (and to create brief release notes); in the past, the
authors and reviewers of each ticket were listed in the release
announcements, so that everyone could check whether his (or someone
else's) name got misspelled etc., at least before a stable release (with
proper release notes) got out.


-leif