D1919: phabricator: specify API tokens per host, rather than per repo
This revision was automatically updated to reflect the committed changes.
Closed by commit rHG4397909f82d3: phabricator: specify API tokens per host,
rather than per repo (authored by tom.prince, committed by ).
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D1919?vs=4960=6730
REVISION DETAIL
https://phab.mercurial-scm.org/D1919
AFFECTED FILES
contrib/phabricator.py
CHANGE DETAILS
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-
"""
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"""
To: tom.prince, #hg-reviewers, durin42
Cc: quark, indygreg, durin42, pulkit, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
durin42 added a comment. (If people are happy enough with that, I'll plan to mail my follow-up as a patch stacked on this one so they can be landed as a pair.) REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1919 To: tom.prince, #hg-reviewers, durin42 Cc: quark, indygreg, durin42, pulkit, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
durin42 added a comment.
Coming back to this. How about we do this:
diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -99,6 +99,17 @@ def urlencodenested(params):
process('', params)
return util.urlreq.urlencode(flatparams)
+def readlegacytoken(repo):
+"""Transitional support for old phabricator tokens.
+
+Remove before the 4.6 release.
+"""
+token = ui.config('phabricator', 'token')
+if token:
+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
@@ -128,8 +139,10 @@ def readurltoken(repo):
break
if not token:
-raise error.Abort(_('Can\'t find conduit token associated to %s')
- % (url,))
+token = readlegacytoken(repo)
+if not token:
+raise error.Abort(_('Can\'t find conduit token associated to
%s')
+ % (url,))
return url, token
for a few weeks to give people a migration path? Can we live with that? I'm
happy to do that as a follow-up.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D1919
To: tom.prince, #hg-reviewers, durin42
Cc: quark, indygreg, durin42, pulkit, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1919: phabricator: specify API tokens per host, rather than per repo
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
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-
"""
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"""
To: tom.prince, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
