1211564 - Prompt to publish from commandline regardless of whether or not the commit message has a reviewer

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P3)

Description

[Andrew Halberstadt [:ahal]]

For some reason, it's only possible to publish a review from the commandline if the commit messages contains 'r=' or 'r?'. I can think of all kinds of workflows that involve pushing a change to mozreview and not flagging a reviewer right away. Personally I like to push my unfinished features to mozreview every once in awhile so I can track my work over time via the slider (and also so other people can follow along).

Requiring a reviewer for command line publishing seems to needlessly inconvenience people who don't want to ask for review right away. Put another way, the web ui doesn't force you to have a reviewer before publishing, so why should the command line?

Comment 1

[Dan Minor [:dminor]]

I'm almost certain we used to force people to add reviewers before publishing in the web ui. This is no longer the case, so it doesn't make sense to do this on the command line, either.

Comment 2

[Mark Côté [:mcote]]

Yeah, we used to require it because Review Board requires at least one person or group to be added to a request before it will publish it.  We now always set a group on gecko review requests, so a reviewer is not technically needed.  As you say, since it's no longer required in the UI, there's no sense in requiring it on the command line.

Comment 3

[Dan Minor [:dminor]]

Attachment: MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r?gps

We allow publishing from the web ui without specifying a reviewer, at least
for some repositories, including gecko. This removes that requirement on the
command line.

This changed the behaviour of many of the tests which assume that we will not
publish commits without reviewers. To preserve the old behaviour a
config option is provided to disable automatic publishing when not desired.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

https://reviewboard.mozilla.org/r/21881/#review19657

::: hgext/reviewboard/client.py:640
(Diff revision 1)
> -        if havereviewers:
> +        if ui.config('reviewboard', 'noautopublish', 'false') != 'true':

There is a configbool() that converts the option to a bool type. It converts a ton of values that aren't "true" and "false." https://selenic.com/repo/hg/file/a38924f7680c/mercurial/util.py#l1940

Could we also change this to "autopublish" to avoid the double negative?

::: hgext/reviewboard/tests/test-auth.t:174
(Diff revision 1)
> -  (review requests lack reviewers; visit review url to assign reviewers and publish these review requests)
> +  (visit review url to publish these review requests so others can see them)

I think we should keep "review requests lack reviewers" as an advisory message.

What do you think?

Comment 5

[Dan Minor [:dminor]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r?gps

We allow publishing from the web ui without specifying a reviewer, at least
for some repositories, including gecko. This removes that requirement on the
command line.

This changed the behaviour of many of the tests which assume that we will not
publish commits without reviewers. To preserve the old behaviour a
config option is provided to disable automatic publishing when not desired.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

https://reviewboard.mozilla.org/r/21881/#review19857

::: hgext/reviewboard/client.py:650
(Diff revision 2)
>              caps = getreviewcaps(remote)
>              if 'publish' in caps:

We implemented an agressive feature negotiation system into the extension so the extension will fail to push if it is out of date. Long story short, this if block will always be true and you can drop the else block below.

::: hgext/reviewboard/tests/helpers.sh:31
(Diff revision 2)
> +# Disable automatic publishing of reviews
> +autopublish = false

configbool() defaults to false, so you don't need this.

::: hgext/reviewboard/tests/test-specify-reviewers.t:535
(Diff revision 2)
> -  review:     http://*:$HGPORT1/r/13 (draft) (glob)
> +  review:     http://172.17.42.1:$HGPORT1/r/13

Dropped a glob

::: hgext/reviewboard/tests/test-specify-reviewers.t:546
(Diff revision 2)
> +  publish these review requests now (Yn)? y
> +  (published review request 12)

This is unexpected. Why did you add autopublish to this test?

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r?gps

We allow publishing from the web ui without specifying a reviewer, at least
for some repositories, including gecko. This removes that requirement on the
command line.

This changed the behaviour of many of the tests which assume that we will not
publish commits without reviewers. To preserve the old behaviour a
config option is provided to disable automatic publishing when not desired.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

https://reviewboard.mozilla.org/r/21881/#review19945

With this patch, we won't prompt to publish unless the user has a config option set. This feels like a regression. We want the prompt to always appear regardless of whether the submission has a reviewer, no?

::: hgext/reviewboard/client.py:667
(Diff revision 3)
> -        if havereviewers:
> +        if ui.configbool('reviewboard', 'autopublish'):

This needs to default to True otherwise we won't prompt to review in interactive mode, right?

Comment 9

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/21881/#review19945

> This needs to default to True otherwise we won't prompt to review in interactive mode, right?

Oops, that slipped by when I switched from noautopublish to autopublish.

Comment 10

[Dan Minor [:dminor]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r?gps

We allow publishing from the web ui without specifying a reviewer, at least
for some repositories, including gecko. This removes that requirement on the
command line.

This changed the behaviour of many of the tests which assume that we will not
publish commits without reviewers. To preserve the old behaviour a
config option is provided to disable automatic publishing when not desired.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

https://reviewboard.mozilla.org/r/21881/#review20175

::: hgext/reviewboard/tests/helpers.sh:27
(Diff revision 4)
> +# Disable automatic publishing
> +autopublish = false

I'm not super thrilled by this because I'd prefer tests test the default behavior in the majority of circumstances. But, yeah, changing this will invalidate pretty much every test and would be a PITA to fix. So this is fine.

::: hgext/reviewboard/tests/test-specify-reviewers.t:515
(Diff revision 4)
>    $ rbmanage list-reviewers 12 --draft

This just changed from not publishing to publishing. I don't think there will be a draft object on this review request. So drop ``--draft``.

Relatedly, it might be a bug that `rbmanage list-reviewers` doesn't error if there is no draft object.

Comment 12

[Dan Minor [:dminor]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

We allow publishing from the web ui without specifying a reviewer, at least
for some repositories, including gecko. This removes that requirement on the
command line.

This changed the behaviour of many of the tests which assume that we will not
publish commits without reviewers. To preserve the old behaviour a
config option is provided to disable automatic publishing when not desired.

Comment 13

[Dan Minor [:dminor]]

Thanks!

I'm going to use this a my test case for autolanding to version-control-tools, so it might not land right away.

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

We allow publishing from the web ui without specifying a reviewer, at least
for some repositories, including gecko. This removes that requirement on the
command line.

This changed the behaviour of many of the tests which assume that we will not
publish commits without reviewers. To preserve the old behaviour a
config option is provided to disable automatic publishing when not desired
which is set by default in helpers.sh.

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8673256 [details]
MozReview Request: Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

Allow publishing from commandline regardless of whether or not the commit message has a reviewer (bug 1211564) r=gps

We allow publishing from the web ui without specifying a reviewer, at least
for some repositories, including gecko. This removes that requirement on the
command line.

This changed the behaviour of many of the tests which assume that we will not
publish commits without reviewers. To preserve the old behaviour a
config option is provided to disable automatic publishing when not desired.
This is set by default in helpers.sh.