Page MenuHomePhabricator

phabricator: fallback reading arcanist config files
Needs RevisionPublic

Authored by philpep on Nov 23 2018, 12:12 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

philpep created this revision.Nov 23 2018, 12:12 PM
philpep added inline comments.Nov 23 2018, 12:15 PM
hgext/phabricator.py
207

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 ?

philpep added inline comments.Nov 23 2018, 12:16 PM
hgext/phabricator.py
181

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

mharbison72 added inline comments.
hgext/phabricator.py
185

pycompat.iswindows

187

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

philpep updated this revision to Diff 14174.Feb 21 2019, 1:08 PM
philpep updated this revision to Diff 14175.Feb 21 2019, 1:16 PM
philpep marked 2 inline comments as done.Feb 21 2019, 1:19 PM

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!

mharbison72 added inline comments.Feb 22 2019, 12:59 PM
hgext/phabricator.py
187

s/.encoding/.environ/ ?

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.

290

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

philpep updated this revision to Diff 14253.Feb 27 2019, 5:48 AM
philpep marked 3 inline comments as done.Feb 27 2019, 5:49 AM
philpep added inline comments.
hgext/phabricator.py
187

Woops... Thanks for catching this!

200

Yes, done.

290

Indeed, I fixed this.

philpep marked 4 inline comments as done.Mar 4 2019, 5:56 PM
Kwan added a subscriber: Kwan.Apr 19 2019, 4:08 AM
Kwan added inline comments.
hgext/phabricator.py
207

I think you could do

url = util.url(conduit_uri)
url.path = None
url = b'%s' % url

Also I think you need to check for phabricator.uri first, then fall back to conduit_uri, see change to phabricator's .arcconfig and mozilla-central's current one (and perhaps rename conduit_uri variable appropriately)

Kwan added inline comments.May 11 2019, 1:55 PM
hgext/phabricator.py
193

Typo: should be arcconfig (two 'c's).
Also no dot for the "/etc/" one according to the docs

207

Oh, and the config.get('hosts', {}).get(uri) would have to use a uri with .path = b'api/'

217–222

Something I realised with some testing is that this requires both .arcconfig and .arcrc, one can't rely on the repo's .arcconfig for the repo settings while using .hg/hgrc for the auth settings (or use hgrc for phab url with .arcrc for auth). Maybe it could instead do the following:

hgrcurl = repo.ui.config(b'phabricator', b'url')
arcurl, arctoken, __ = readarcconfig(repo)
url = hgrcurl or arcurl
if not url:
    [etc...]

res = [etc...]
hgrctoken = None

if res:
    [etc...]
    hgrctoken = auth.get(b'token')
token = hgrctoken or arctoken
if not token:
    [etc...]

That way any mix of settings in hgrc or .arcconfig/.arcrc would work, with hgrc having precedence.

Kwan added inline comments.May 12 2019, 7:04 AM
hgext/phabricator.py
181

Looks like annotating it with @util.cachefunc is enough?

217–222

Although, this wouldn't work with changing phab instances in hgrc, and using .arcrc for the token, since readarcconfig would return the token for the original URL. An alternative would be having readarcconfig return a dict mapping hosts to tokens (with '/api/' stripped from the hosts), and then readurltoken does token = hgrctoken or arctokens.get(url).

durin42 requested changes to this revision.May 15 2019, 2:08 PM
durin42 added a subscriber: durin42.

I see at least one outstanding comment that points out a typo. I'm generally in favor of this though, so please let us know when it's time to take another look.

This revision now requires changes to proceed.May 15 2019, 2:08 PM

Thanks for reviewing @Kwan !

Actually I think parsing/merging arcconfig files will be quite hard to maintain because they are not fully documented and subject to changes.

For my needs, just having hg being able to get phabricator.url and phabricator.callsign from .arcconfig in the repo root is enough. Then I just need to match this with hg auth config.
I'll try to have a smaller patch not trying to read other files than .arcconfig and not reading the token.