Patchwork D1919: phabricator: specify API tokens per host, rather than per repo

login
register
mail settings
Submitter phabricator
Date Jan. 20, 2018, 9:52 a.m.
Message ID <differential-rev-PHID-DREV-blst57rdqf5qzgjutffi-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27002/
State New
Headers show

Comments

phabricator - Jan. 20, 2018, 9:52 a.m.
tom.prince created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS




To: tom.prince, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 20, 2018, 9:53 a.m.
tom.prince added a comment.


  I'm not sure what the compatibility policy for `contrib/` files is. This just *changes* the format for auth data, but it wouldn't be hard to support the old format too.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 20, 2018, 10:26 a.m.
pulkit added a comment.


  Hey, the code freeze for new release of Mercurial started last night. We only accept bug fixes during code freeze, so this has to wait till 1st feb. Refer: https://www.mercurial-scm.org/wiki/TimeBasedReleasePlan#Code_Freeze

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Jan. 20, 2018, 2:53 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1919#32356, @tom.prince wrote:
  
  > I'm not sure what the compatibility policy for `contrib/` files is. This just *changes* the format for auth data, but it wouldn't be hard to support the old format too.
  
  
  I'm not inclined to worry about it, we'll just post a notice on the list and mark this as (BC) and go with it.
  
  Probably will come back to this on Feb 2 or so per the freeze.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers
Cc: durin42, pulkit, mercurial-devel
phabricator - Feb. 1, 2018, 11:34 p.m.
durin42 accepted this revision as: durin42.
durin42 added a comment.


  I like this, and I'm fine with the backwards incompatible change in contrib. How do others feel?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: durin42, pulkit, mercurial-devel
phabricator - Feb. 1, 2018, 11:41 p.m.
indygreg added a comment.


  In https://phab.mercurial-scm.org/D1919#33462, @durin42 wrote:
  
  > I like this, and I'm fine with the backwards incompatible change in contrib. How do others feel?
  
  
  I'm fine with BC in contrib.
  
  One thing to bikeshed is if the `[phabricator]` section is the best place for credentials. We already have `[auth]` for storing credentials. But as long as we don't care about breaking BC, this patch is fine for now.
  
  Another thing to consider is reading things from `.arcconfig` files and reading the API token however Arcanist does it (as the comment in this file suggests).
  
  FWIW Mozilla will likely start leaning on this extension pretty soon. Look for a push from us to move it to core before the next release.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: indygreg, durin42, pulkit, mercurial-devel
phabricator - Feb. 1, 2018, 11:43 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1919#33517, @indygreg wrote:
  
  > FWIW Mozilla will likely start leaning on this extension pretty soon. Look for a push from us to move it to core before the next release.
  
  
  I've had a...slightly bonkers idea around using docker to stand up a test phabricator server in a .t-test so we can test this extension. I think it might work.
  
  Most of the reason we dumped this in contrib and not hgext was that it's untested.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: indygreg, durin42, pulkit, mercurial-devel
phabricator - Feb. 1, 2018, 11:47 p.m.
indygreg added a comment.


  In https://phab.mercurial-scm.org/D1919#33519, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D1919#33517, @indygreg wrote:
  >
  > > FWIW Mozilla will likely start leaning on this extension pretty soon. Look for a push from us to move it to core before the next release.
  >
  >
  > I've had a...slightly bonkers idea around using docker to stand up a test phabricator server in a .t-test so we can test this extension. I think it might work.
  >
  > Most of the reason we dumped this in contrib and not hgext was that it's untested.
  
  
  We could also use Betamax for HTTP record and replay. Then we could create a Python script to run Docker to perform the recording of the HTTP requests we care about. That would remove Docker from test run-time requirements. Having used Docker for testing against actual services, I've learned the hard way that Docker doesn't scale well. Once you start running tests concurrently, Docker container start and stop overhead quick makes test run times unbearable. And there's intermittent failures in Docker :(

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: indygreg, durin42, pulkit, mercurial-devel
phabricator - Feb. 2, 2018, 7:02 a.m.
quark added a comment.


  > Another thing to consider is reading things from .arcconfig files and reading the API token however Arcanist does it (as the comment in this file suggests).
  
  That was my original plan - in `readurltoken`, fallback to `~/.arcrc` and `.arcconfig` if hgrc does not have enough information. I didn't implement it though.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: quark, indygreg, durin42, pulkit, mercurial-devel
phabricator - Feb. 15, 2018, 1:45 a.m.
durin42 added a comment.


  Sorry, this slipped through the cracks. If we think we want to start reading .arcconfig, should we just go straight to that and discard our own auth config area, and incur a BC only once?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: quark, indygreg, durin42, pulkit, mercurial-devel
phabricator - Feb. 15, 2018, 3:21 a.m.
quark added a comment.


  In https://phab.mercurial-scm.org/D1919#37546, @durin42 wrote:
  
  > Sorry, this slipped through the cracks. If we think we want to start reading .arcconfig, should we just go straight to that and discard our own auth config area, and incur a BC only once?
  
  
  One good thing about hgrc is, editing hgrc is human-friendly. I think editing `.arcrc` is not. If we decided to use `.arcrc`, that means there needs to be some program that edits it. It could be `arc`, but that's not friendly for people who don't want PHP.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: quark, indygreg, durin42, pulkit, mercurial-devel
phabricator - Feb. 16, 2018, 6:59 p.m.
tom.prince added a comment.


  Personally, I don't have arc installed, so I'd prefer putting the auth information in mercurial rather than arc specific places (this is what is stored in `.arcrc` I think). It would make sense to support `.arcrc` as well, though.
  
  It would make sense to read the URL and call-sign from `.arcconfig`, which is a file in the repo, if it is provided.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: quark, indygreg, durin42, pulkit, mercurial-devel
phabricator - Feb. 16, 2018, 9:47 p.m.
durin42 added a comment.


  I generally agree we should support a non-arc option, I'm happy to not have arc on my machines anymore.
  
  I'll meditate on this over the long weekend.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1919

To: tom.prince, #hg-reviewers, durin42
Cc: quark, indygreg, durin42, pulkit, mercurial-devel

Patch

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -33,6 +33,11 @@ 
     # if you need to specify advanced options that is not easily supported by
     # the internal library.
     curlcmd = curl --connect-timeout 2 --retry 3 --silent
+
+    [phabricator.auth]
+    example.url = https://phab.example.com/
+    # API token. Get it from https://$HOST/conduit/login/
+    example.token = cli-xxxxxxxxxxxxxxxxxxxxxxxxxxxx
 """
 
 from __future__ import absolute_import
@@ -100,14 +105,33 @@ 
     Currently read from [phabricator] config section. In the future, it might
     make sense to read from .arcconfig and .arcrc as well.
     """
-    values = []
-    section = 'phabricator'
-    for name in ['url', 'token']:
-        value = repo.ui.config(section, name)
-        if not value:
-            raise error.Abort(_('config %s.%s is required') % (section, name))
-        values.append(value)
-    return values
+    url = repo.ui.config('phabricator', 'url')
+    if not url:
+        raise error.Abort(_('config %s.%s is required')
+                          % ('phabricator', 'url'))
+
+    groups = {}
+    for key, val in repo.ui.configitems('phabricator.auth'):
+        if '.' not in key:
+            repo.ui.warn(_("ignoring invalid [phabricator.auth] key '%s'\n")
+                         % key)
+            continue
+        group, setting = key.rsplit('.', 1)
+        groups.setdefault(group, {})[setting] = val
+
+    token = None
+    for group, auth in groups.iteritems():
+        if url != auth.get('url'):
+            continue
+        token = auth.get('token')
+        if token:
+            break
+
+    if not token:
+        raise error.Abort(_('Can\'t find conduit token associated to %s')
+                          % (url,))
+
+    return url, token
 
 def callconduit(repo, name, params):
     """call Conduit API, params is a dict. return json.loads result, or None"""