Patchwork phabricator: drop support for the deprecated `phabricator.token` config

login
register
mail settings
Submitter Matt Harbison
Date May 12, 2018, 1:16 a.m.
Message ID <35b230be2610a6ba4b6d.1526087769@Envy>
Download mbox | patch
Permalink /patch/31524/
State Accepted
Headers show

Comments

Matt Harbison - May 12, 2018, 1:16 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1526004793 14400
#      Thu May 10 22:13:13 2018 -0400
# Node ID 35b230be2610a6ba4b6d6b60e228ec4a8ddfdfae
# Parent  3ac950cd597857af7797f75bb3a6b4c3694cc79c
phabricator: drop support for the deprecated `phabricator.token` config
Matt Harbison - May 12, 2018, 1:31 a.m.
On Fri, 11 May 2018 21:16:09 -0400, Matt Harbison <mharbison72@gmail.com>  
wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1526004793 14400
> #      Thu May 10 22:13:13 2018 -0400
> # Node ID 35b230be2610a6ba4b6d6b60e228ec4a8ddfdfae
> # Parent  3ac950cd597857af7797f75bb3a6b4c3694cc79c
> phabricator: drop support for the deprecated `phabricator.token` config

BTW, are there any plans to move this into the bundled extensions?

I dabbled with Reviewboard this week, but I like how this can download a  
series, remember what review a commit is associated with, etc.  Bundling  
it would be nice for juggling different versions of Mercurial.  But  
Augie's comment on the bug tracker[1] made me think that maybe it's too  
hard to support properly.

[1] https://bz.mercurial-scm.org/show_bug.cgi?id=5679#c5
Yuya Nishihara - May 12, 2018, 2:28 a.m.
On Fri, 11 May 2018 21:16:09 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1526004793 14400
> #      Thu May 10 22:13:13 2018 -0400
> # Node ID 35b230be2610a6ba4b6d6b60e228ec4a8ddfdfae
> # Parent  3ac950cd597857af7797f75bb3a6b4c3694cc79c
> phabricator: drop support for the deprecated `phabricator.token` config

Queued, thanks.
Yuya Nishihara - May 12, 2018, 2:51 a.m.
On Fri, 11 May 2018 21:31:48 -0400, Matt Harbison wrote:
> > phabricator: drop support for the deprecated `phabricator.token` config
> 
> BTW, are there any plans to move this into the bundled extensions?

Perhaps someone will have to spend some time on making it testable.

FWIW, the new config section [phabricator.auth] is incompatible with the
--config=section.name=value syntax. We'll need to rename it.
Matt Harbison - May 12, 2018, 3:01 a.m.
On Fri, 11 May 2018 22:51:27 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Fri, 11 May 2018 21:31:48 -0400, Matt Harbison wrote:
>> > phabricator: drop support for the deprecated `phabricator.token`  
>> config
>>
>> BTW, are there any plans to move this into the bundled extensions?
>
> Perhaps someone will have to spend some time on making it testable.
>
> FWIW, the new config section [phabricator.auth] is incompatible with the
> --config=section.name=value syntax. We'll need to rename it.

I wonder why it wasn't just folded into the existing [auth] config  
section, and maybe rename 'token' to 'password'.
Gregory Szorc - May 12, 2018, 3:44 a.m.
On Fri, May 11, 2018 at 8:01 PM, Matt Harbison <mharbison72@gmail.com>
wrote:

> On Fri, 11 May 2018 22:51:27 -0400, Yuya Nishihara <yuya@tcha.org> wrote:
>
> On Fri, 11 May 2018 21:31:48 -0400, Matt Harbison wrote:
>>
>>> > phabricator: drop support for the deprecated `phabricator.token` config
>>>
>>> BTW, are there any plans to move this into the bundled extensions?
>>>
>>
>> Perhaps someone will have to spend some time on making it testable.
>>
>> FWIW, the new config section [phabricator.auth] is incompatible with the
>> --config=section.name=value syntax. We'll need to rename it.
>>
>
> I wonder why it wasn't just folded into the existing [auth] config
> section, and maybe rename 'token' to 'password'.


+1 to moving it into [auth].

Since Phabricator's authentication is supplemental to HTTP authentication
(which is what the "password" config options would be for), we should
probably name the option "phabtoken" or something.


>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - May 12, 2018, 3:18 p.m.
> On May 11, 2018, at 9:31 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> BTW, are there any plans to move this into the bundled extensions?

Broadly, yes. The big blocker is having a testing story, because right now the “best” options I’ve heard are:

1) use a docker container to spin up an entire phabricator for testing
2) some kind replay mechanism on http traffic so we can verify the right requests are still being made

I think we’re all very in favor of (2), but haven’t had the time.

> I dabbled with Reviewboard this week, but I like how this can download a series, remember what review a commit is associated with, etc.  Bundling it would be nice for juggling different versions of Mercurial.  But Augie's comment on the bug tracker[1] made me think that maybe it's too hard to support properly.

I haven’t used reviewboard in a while, but Mozilla is moving from RB to Phab, which was enough to convince me to not go back and look at RB again.
Matt Harbison - May 12, 2018, 3:46 p.m.
On Sat, 12 May 2018 11:18:18 -0400, Augie Fackler <raf@durin42.com> wrote:

>
>
>> On May 11, 2018, at 9:31 PM, Matt Harbison <mharbison72@gmail.com>  
>> wrote:
>>
>> I dabbled with Reviewboard this week, but I like how this can download  
>> a series, remember what review a commit is associated with, etc.   
>> Bundling it would be nice for juggling different versions of  
>> Mercurial.  But Augie's comment on the bug tracker[1] made me think  
>> that maybe it's too hard to support properly.
>
> I haven’t used reviewboard in a while, but Mozilla is moving from RB to  
> Phab, which was enough to convince me to not go back and look at RB  
> again.

OK, good info, thanks.  I really don't have any experience with either,  
other than the occasional review here.  Another group at my company uses  
RB, so I'm trying to figure out if there are technical benefits to Phab,  
without sinking a bunch of time into it.  (I'd consider a bundled  
extension that doesn't need external maintenance, plus evolve support, to  
be a huge benefit.)

Have there been administrative or user issues?  It seems to be running  
pretty smoothly, looking at it from a distance.
Augie Fackler - May 12, 2018, 5:50 p.m.
> On May 12, 2018, at 11:46 AM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Sat, 12 May 2018 11:18:18 -0400, Augie Fackler <raf@durin42.com> wrote:
> 
>> 
>> 
>>> On May 11, 2018, at 9:31 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>> 
>>> I dabbled with Reviewboard this week, but I like how this can download a series, remember what review a commit is associated with, etc.  Bundling it would be nice for juggling different versions of Mercurial.  But Augie's comment on the bug tracker[1] made me think that maybe it's too hard to support properly.
>> 
>> I haven’t used reviewboard in a while, but Mozilla is moving from RB to Phab, which was enough to convince me to not go back and look at RB again.
> 
> OK, good info, thanks.  I really don't have any experience with either, other than the occasional review here.  Another group at my company uses RB, so I'm trying to figure out if there are technical benefits to Phab, without sinking a bunch of time into it.  (I'd consider a bundled extension that doesn't need external maintenance, plus evolve support, to be a huge benefit.)
> 
> Have there been administrative or user issues?  It seems to be running pretty smoothly, looking at it from a distance.

The biggest user issue is (to my knowledge) a peculiarity of our review process: we expect submissions to just be sent in general to the list, and reviewers to pick them up. junw built us a custom UI that helps a fair amount, visible at https://phab.mercurial-scm.org/yadda/, code at https://bitbucket.org/quark-zju/phab-yadda/

I’d be absurdly happy if we can move the extension to hgext from contrib. I’m a little busy with family right now, but I can promise review time...
Gregory Szorc - May 12, 2018, 6:53 p.m.
On Sat, May 12, 2018 at 8:18 AM, Augie Fackler <raf@durin42.com> wrote:

>
>
> > On May 11, 2018, at 9:31 PM, Matt Harbison <mharbison72@gmail.com>
> wrote:
> >
> > BTW, are there any plans to move this into the bundled extensions?
>
> Broadly, yes. The big blocker is having a testing story, because right now
> the “best” options I’ve heard are:
>
> 1) use a docker container to spin up an entire phabricator for testing
> 2) some kind replay mechanism on http traffic so we can verify the right
> requests are still being made
>
> I think we’re all very in favor of (2), but haven’t had the time.
>
> > I dabbled with Reviewboard this week, but I like how this can download a
> series, remember what review a commit is associated with, etc.  Bundling it
> would be nice for juggling different versions of Mercurial.  But Augie's
> comment on the bug tracker[1] made me think that maybe it's too hard to
> support properly.
>
> I haven’t used reviewboard in a while, but Mozilla is moving from RB to
> Phab, which was enough to convince me to not go back and look at RB again.
>

Without saying that Mozilla's decision was wrong or misguided, speaking as
a Mozillian, I feel that modern versions of Reviewboard (which has native
support for a series of commits) are fine. Both Reviewboard and Phabricator
have their own set of warts. Pick your poison. Mozilla's decision had more
to do with special requirements and exigent circumstances than Reviewboard
versus Phabricator per se. It's complicated. I wouldn't read too much into
Mozilla's decision to switch from Reviewboard to Phabricator.

That being said, I *really* like Phabricator's model of grouping commits
into a series. Having a loose coupling makes it easier to make piecemeal
progress towards landing commits. Contrast with other review tools which
put an emphasis on atomically landing a group of commits only once they are
all ready. There are various benefits to landing commits as soon as they
are ready (avoiding bit rot, getting feedback from other users sooner,
people feel better when making progress, etc). Phabricator's very loose
grouping of commits allows it to support incremental landing workflows
better than other code review tools. Unfortunately, some VCS tools inhibit
users from realizing these benefits (e.g. difficulty rebasing a partially
landed "branch"). Mercurial with obsolescence markers works incredibly well
for this workflow.
Julien Cristau - May 12, 2018, 9:34 p.m.
Hi,

On Fri, May 11, 2018 at 21:16:09 -0400, Matt Harbison wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1526004793 14400
> #      Thu May 10 22:13:13 2018 -0400
> # Node ID 35b230be2610a6ba4b6d6b60e228ec4a8ddfdfae
> # Parent  3ac950cd597857af7797f75bb3a6b4c3694cc79c
> phabricator: drop support for the deprecated `phabricator.token` config
> 
You might want to update
https://www.mercurial-scm.org/wiki/Phabricator#Setting_up_hg for this
change.

Thanks,
Julien
Matt Harbison - May 12, 2018, 9:57 p.m.
On Sat, 12 May 2018 17:34:33 -0400, Julien Cristau <jcristau@debian.org>  
wrote:

> Hi,
>
> On Fri, May 11, 2018 at 21:16:09 -0400, Matt Harbison wrote:
>
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1526004793 14400
>> #      Thu May 10 22:13:13 2018 -0400
>> # Node ID 35b230be2610a6ba4b6d6b60e228ec4a8ddfdfae
>> # Parent  3ac950cd597857af7797f75bb3a6b4c3694cc79c
>> phabricator: drop support for the deprecated `phabricator.token` config
>>
> You might want to update
> https://www.mercurial-scm.org/wiki/Phabricator#Setting_up_hg for this
> change.

OK, thanks.  I didn't know about this.

I'll make a stab at it after the move to [auth] gets taken, so I only have  
to mess with this once.  I'm wondering how to (or if we should) document  
the now 3 ways of doing this, depending on the version.
Matt Harbison - May 12, 2018, 10:22 p.m.
On Sat, 12 May 2018 14:53:38 -0400, Gregory Szorc  
<gregory.szorc@gmail.com> wrote:

> On Sat, May 12, 2018 at 8:18 AM, Augie Fackler <raf@durin42.com> wrote:
>
>>
>>
>> > On May 11, 2018, at 9:31 PM, Matt Harbison <mharbison72@gmail.com>
>> wrote:
>> >
>> > BTW, are there any plans to move this into the bundled extensions?
>>
>> Broadly, yes. The big blocker is having a testing story, because right  
>> now
>> the “best” options I’ve heard are:
>>
>> 1) use a docker container to spin up an entire phabricator for testing
>> 2) some kind replay mechanism on http traffic so we can verify the right
>> requests are still being made
>>
>> I think we’re all very in favor of (2), but haven’t had the time.
>>
>> > I dabbled with Reviewboard this week, but I like how this can  
>> download a
>> series, remember what review a commit is associated with, etc.   
>> Bundling it
>> would be nice for juggling different versions of Mercurial.  But Augie's
>> comment on the bug tracker[1] made me think that maybe it's too hard to
>> support properly.
>>
>> I haven’t used reviewboard in a while, but Mozilla is moving from RB to
>> Phab, which was enough to convince me to not go back and look at RB  
>> again.
>>
>
> Without saying that Mozilla's decision was wrong or misguided, speaking  
> as
> a Mozillian, I feel that modern versions of Reviewboard (which has native
> support for a series of commits) are fine. Both Reviewboard and  
> Phabricator
> have their own set of warts. Pick your poison. Mozilla's decision had  
> more
> to do with special requirements and exigent circumstances than  
> Reviewboard
> versus Phabricator per se. It's complicated. I wouldn't read too much  
> into
> Mozilla's decision to switch from Reviewboard to Phabricator.

Did you roll your own RB extension, or use one of the two(?) floating  
around on bitbucket?  The experimental server I used was setup last fall,  
and that server didn't look like it supported a series.  (Though the  
extension only supported --outgoing, and not revsets.)

If it was a custom extension, did it provide similar functionality as this  
Phab extension, like downloading the patch/series, knowing what review to  
update, evolve support, and so forth?

Patch

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -21,10 +21,6 @@  Config::
     # Phabricator URL
     url = https://phab.example.com/
 
-    # API token. Get it from https://$HOST/conduit/login/
-    # Deprecated: see [phabricator.auth] below
-    #token = cli-xxxxxxxxxxxxxxxxxxxxxxxxxxxx
-
     # Repo callsign. If a repo has a URL https://$HOST/diffusion/FOO, then its
     # callsign is "FOO".
     callsign = FOO
@@ -104,21 +100,6 @@  def urlencodenested(params):
     process('', params)
     return util.urlreq.urlencode(flatparams)
 
-printed_token_warning = False
-
-def readlegacytoken(repo):
-    """Transitional support for old phabricator tokens.
-
-    Remove before the 4.6 release.
-    """
-    global printed_token_warning
-    token = repo.ui.config('phabricator', 'token')
-    if token and not printed_token_warning:
-        printed_token_warning = True
-        repo.ui.warn(_('phabricator.token is deprecated - please '
-                       'migrate to the phabricator.auth section.\n'))
-    return token
-
 def readurltoken(repo):
     """return conduit url, token and make sure they exist
 
@@ -148,10 +129,8 @@  def readurltoken(repo):
             break
 
     if not token:
-        token = readlegacytoken(repo)
-        if not token:
-            raise error.Abort(_('Can\'t find conduit token associated to %s')
-                              % (url,))
+        raise error.Abort(_('Can\'t find conduit token associated to %s')
+                          % (url,))
 
     return url, token