This is an archive of the discontinued Mercurial Phabricator instance.

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.

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

In 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.

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

In 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.

In 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.

In D1919#33519, @durin42 wrote:

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

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?

In 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.

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

durin42 added a child revision: D2703: phabricator: follow-up phab auth improvements with backwards compat mode.Mar 6 2018, 2:01 PM

Closed by commit rHG4397909f82d3: phabricator: specify API tokens per host, rather than per repo (authored by tom.prince). · Explain WhyMar 8 2018, 2:19 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			contrib/phabricator.py (40 lines)

	Status	Author	Revision
	Closed	durin42	D2703 phabricator: follow-up phab auth improvements with backwards compat mode
	Closed	tom.prince	D1919 phabricator: specify API tokens per host, rather than per repo

Diff 6730

contrib/phabricator.py

	# callsign is "FOO".			# callsign is "FOO".
	callsign = FOO			callsign = FOO

	# curl command to use. If not set (default), use builtin HTTP library to			# curl command to use. If not set (default), use builtin HTTP library to
	# communicate. If set, use the specified curl command. This could be useful			# communicate. If set, use the specified curl command. This could be useful
	# if you need to specify advanced options that is not easily supported by			# if you need to specify advanced options that is not easily supported by
	# the internal library.			# the internal library.
	curlcmd = curl --connect-timeout 2 --retry 3 --silent			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			from __future__ import absolute_import

	import itertools			import itertools
	import json			import json
	import operator			import operator
	import re			import re
	return util.urlreq.urlencode(flatparams)			return util.urlreq.urlencode(flatparams)

	def readurltoken(repo):			def readurltoken(repo):
	"""return conduit url, token and make sure they exist			"""return conduit url, token and make sure they exist

	Currently read from [phabricator] config section. In the future, it might			Currently read from [phabricator] config section. In the future, it might
	make sense to read from .arcconfig and .arcrc as well.			make sense to read from .arcconfig and .arcrc as well.
	"""			"""
	values = []			url = repo.ui.config('phabricator', 'url')
	section = 'phabricator'			if not url:
	for name in ['url', 'token']:			raise error.Abort(_('config %s.%s is required')
	value = repo.ui.config(section, name)			% ('phabricator', 'url'))
	if not value:
	raise error.Abort(_('config %s.%s is required') % (section, name))			groups = {}
	values.append(value)			for key, val in repo.ui.configitems('phabricator.auth'):
	return values			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):			def callconduit(repo, name, params):
	"""call Conduit API, params is a dict. return json.loads result, or None"""			"""call Conduit API, params is a dict. return json.loads result, or None"""
	host, token = readurltoken(repo)			host, token = readurltoken(repo)
	url, authinfo = util.url('/'.join([host, 'api', name])).authinfo()			url, authinfo = util.url('/'.join([host, 'api', name])).authinfo()
	repo.ui.debug('Conduit Call: %s %s\n' % (url, params))			repo.ui.debug('Conduit Call: %s %s\n' % (url, params))
	params = params.copy()			params = params.copy()
	params['api.token'] = token			params['api.token'] = token

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	4960		Jan 20 2018, 4:52 AM	★	★
Diff 2	6730	rHG4397909f82d37cd133ec2f1d0f34c93a72b3f6d9	Jan 20 2018, 4:41 AM	★	★

This is an archive of the discontinued Mercurial Phabricator instance.

phabricator: specify API tokens per host, rather than per repoClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 6730

contrib/phabricator.py

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

Revision Contents
Changeset List