Page MenuHomePhabricator

phabricator: specify API tokens per host, rather than per repo
ClosedPublic

Authored by tom.prince on Jan 20 2018, 4:52 AM.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tom.prince created this revision.Jan 20 2018, 4:52 AM

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.

pulkit added a subscriber: pulkit.Jan 20 2018, 5:26 AM

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

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.

durin42 accepted this revision as: durin42.Feb 1 2018, 6:34 PM

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

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.

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.

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 :(

quark added a subscriber: quark.Feb 2 2018, 2:02 AM

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.

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?

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.

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.

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.

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.

(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.)

This revision was automatically updated to reflect the committed changes.