Patchwork D5299: phabricator: fallback reading arcanist config files

login
register
mail settings
Submitter phabricator
Date Nov. 23, 2018, 5:13 p.m.
Message ID <differential-rev-PHID-DREV-dj57pl63jjgmeclpeicu-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/36749/
State New
Headers show

Comments

phabricator - Nov. 23, 2018, 5:13 p.m.
philpep created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This allow the phabricator extension to read arc config files to auto-configure
  url, token and callsign.
  
  We use it as a fallback when phabricator.url or phabricator.callsign aren't
  defined.
  
  This allow to configure conduit_uri and repository.callsign in a tracked
  .arcconfig json file in the root of the repository, so users having lot of
  small repositories in phabricator doesn't need to configure .hg/hgrc after a
  fresh clone.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/phabricator.py

CHANGE DETAILS




To: philpep, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 23, 2018, 5:15 p.m.
philpep added inline comments.

INLINE COMMENTS

> phabricator.py:201
> +    if conduit_uri is not None:
> +        token = config.get('hosts', {}).get(conduit_uri, {}).get('token')
> +    url = conduit_uri.rstrip('/api/')

HINT: This doesn't work for current mercurial config because "arc install-certificates" add a trailing "/" to conduit_uri and our .arcconfig doesn't have this trailing slash.
Any idea how to handle this properly ?

REPOSITORY
  rHG Mercurial

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

To: philpep, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 23, 2018, 5:16 p.m.
philpep added inline comments.

INLINE COMMENTS

> phabricator.py:175
> +def readarcconfig(repo):
> +    """Return url, token, callsign read from arcanist config files
> +

I'd be nice to cache the result of `readarcconfig` but I don't known how to implement this, any suggestion ?

REPOSITORY
  rHG Mercurial

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

To: philpep, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 24, 2018, 5:31 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> phabricator.py:179
> +    """
> +    if os.name == 'nt':
> +        paths = [

pycompat.iswindows

> phabricator.py:181
> +        paths = [
> +            os.path.join(os.environ['ProgramData'],
> +                         'Phabricator',

I think there’s something for environment variables in encoding (or maybe procutil- I don’t have the code in front of me).

Also, is there a way to use vfs here?  I think we’re trying to avoid direct os.path usage.

https://www.mercurial-scm.org/wiki/WindowsUTF8Plan

REPOSITORY
  rHG Mercurial

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

To: philpep, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - Feb. 21, 2019, 6:19 p.m.
philpep marked 2 inline comments as done.
philpep added a comment.


  Thanks for the review and sorry for taking so long time to update the patch... I made changes to use encoding.environ and vfs like you suggested.
  Please let me known what I can do to introduce this (or at least part of) this feature, thanks!

REPOSITORY
  rHG Mercurial

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

To: philpep, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - Feb. 22, 2019, 5:59 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> phabricator.py:187
> +        paths = [
> +            vfsmod.vfs(encoding.encoding['ProgramData']).join(
> +                 'Phabricator', 'Arcanist', 'config'),

s/.encoding/.environ/ ?

> phabricator.py:200
> +        if vfsmod.vfs(path).exists():
> +            with open(path, 'rb') as f:
> +                config.update(json.load(f))

Should this be using vfs to open, instead of raw open?  Using the vfs layer allows the class that provides posix-like functionality on Windows to be used.

> phabricator.py:290
> +        if not callsign:
> +            return callsign
>      query = callconduit(repo, b'diffusion.repository.search',

It might be clearer to return None, since the function is to fetch the repoid.

REPOSITORY
  rHG Mercurial

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

To: philpep, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - Feb. 27, 2019, 10:49 a.m.
philpep marked 3 inline comments as done.
philpep added inline comments.

INLINE COMMENTS

> mharbison72 wrote in phabricator.py:187
> s/.encoding/.environ/ ?

Woops... Thanks for catching this!

> mharbison72 wrote in phabricator.py:200
> Should this be using vfs to open, instead of raw open?  Using the vfs layer allows the class that provides posix-like functionality on Windows to be used.

Yes, done.

> mharbison72 wrote in phabricator.py:290
> It might be clearer to return None, since the function is to fetch the repoid.

Indeed, I fixed this.

REPOSITORY
  rHG Mercurial

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

To: philpep, #hg-reviewers
Cc: mharbison72, mercurial-devel

Patch

diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -37,14 +37,18 @@ 
 
     # API token. Get it from https://$HOST/conduit/login/
     example.phabtoken = cli-xxxxxxxxxxxxxxxxxxxxxxxxxxxx
+
+As a fallback, read config from arc config files (.arcconfig, ~/.arcrc and
+/etc/arcconfig)
 """
 
 from __future__ import absolute_import
 
 import itertools
 import json
 import operator
 import re
+import os
 
 from mercurial.node import bin, nullid
 from mercurial.i18n import _
@@ -167,16 +171,51 @@ 
     process(b'', params)
     return util.urlreq.urlencode(flatparams)
 
+def readarcconfig(repo):
+    """Return url, token, callsign read from arcanist config files
+
+    This read and merge content of /etc/arcconfig, ~/.arcrc and .arconfig.
+    """
+    if os.name == 'nt':
+        paths = [
+            os.path.join(os.environ['ProgramData'],
+                         'Phabricator',
+                         'Arcanist',
+                         'config'),
+            os.path.join(os.environ['AppData'], '.arcrc'),
+        ]
+    else:
+        paths = [
+            os.path.join('/etc', 'arcconfig'),
+            os.path.join(os.path.expanduser('~'), '.arcrc'),
+            os.path.join(repo.root, '.arcconfig'),
+        ]
+    config = {}
+    for path in paths:
+        if os.path.exists(path):
+            with open(path, 'rb') as f:
+                config.update(json.load(f))
+    callsign = config.get('repository.callsign')
+    conduit_uri = config.get('conduit_uri', config.get('config', {}).get('default'))
+    if conduit_uri is not None:
+        token = config.get('hosts', {}).get(conduit_uri, {}).get('token')
+    url = conduit_uri.rstrip('/api/')
+    return url, token, callsign
+
 def readurltoken(repo):
     """return conduit url, token and make sure they exist
 
-    Currently read from [auth] config section. In the future, it might
-    make sense to read from .arcconfig and .arcrc as well.
+    Currently read from [auth] config section and fallback to reading arc
+    config files.
     """
     url = repo.ui.config(b'phabricator', b'url')
     if not url:
-        raise error.Abort(_(b'config %s.%s is required')
-                          % (b'phabricator', b'url'))
+        url, token, __ = readarcconfig(repo)
+        if not url or not token:
+            raise error.Abort(_(b'unable to read phabricator conduit url and '
+                                b'token from config %s.%s or from arc config '
+                                b'files') % (b'phabricator', b'url'))
+        return url, token
 
     res = httpconnectionmod.readauthforuri(repo.ui, url, util.url(url).user)
     token = None
@@ -241,7 +280,9 @@ 
         return repophid
     callsign = repo.ui.config(b'phabricator', b'callsign')
     if not callsign:
-        return None
+        __, __, callsign = readarcconfig(repo)
+        if not callsign:
+            return callsign
     query = callconduit(repo, b'diffusion.repository.search',
                         {b'constraints': {b'callsigns': [callsign]}})
     if len(query[r'data']) == 0: