552370 - Add qexportbz command to qimportbz

Status: RESOLVED INVALID

(Developer Services :: Mercurial: qimportbz, enhancement)

Description

[Atul Varma [:atul]]

qimportbz is great for importing patches from Bugzilla, but it'd be great to be able to export patches back to Bugzilla as well. I've written an extension that does this here:

  http://hg.mozilla.org/users/avarma_mozilla.com/qexportbz/

Will turn this into a patch for qimportbz so folks don't have to get two separate HG extensions.

Comment 1

[Atul Varma [:atul]]

Attachment: patch

Comment 2

[Atul Varma [:atul]]

Since the code for this patch was originally its own hg extension, there's a few oddities here:

* While qimportbz talks to bugzilla directly, this patch uses Gerv's REST API [1]. Because the REST API is actually a separate app that proxies requests to a "real" bugzilla instance, this means users have to provide two separate servers to qimportbz, which isn't ideal. Though at least the defaults work with Mozilla's Bugzilla.

* The configuration variable 'bugzilla_rest_api_url' is inconsistent with the 'bugzilla' config variable in that the former is a URL and the latter is a hostname. However, my local bugzilla REST API proxy operates over a secure virtual network over HTTP--though it connects to bugzilla.mozilla.org over https--while the default REST API proxy operates over https. So there needs to be a way to specify the scheme there.

* Not sure if it's a good idea or not to let people put their plaintext bugzilla password in their .hgrc. We do let them pass it through the commandline and over interactive console.

* Kind of weird that an extension called qimportbz comes with a command called qexportbz... :P

Also, I've only tested this patch on HG 1.3.1.

[1] https://wiki.mozilla.org/Bugzilla:REST_API

Comment 3

[Atul Varma [:atul]]

Also, this extension tries to infer what bug number the patch belongs to by essentially expecting the patch to be named "bug-XXXXX.diff/.patch" or expecting there to be a patch message at the top of the file with the default format that qimportbz supplies when importing from Bugzilla. However, I should probably allow an optional parameter to be passed in which allows the user to specify the bug id manually, in case it can't be inferred (or the user wants to override it).

There is also currently no way for the user to provide a comment along with the patch; they basically are only able to set the patch's description. As far as I can tell from the REST API, we'd have to separately submit a comment apart from the patch--the two can't be provided atomically like they can with the web interface. I could easily be wrong though.

Comment 4

[Atul Varma [:atul]]

Oh, another thing about this patch is that it has a dependency on either simplejson or Python 2.6 (which comes with a 'json' module). This is because the REST API uses JSON.

Comment 5

[Rob Arnold [:robarnold]]

Comment on attachment 432492 [details] [diff] [review]
patch

>+DEFAULT_BZ_REST_API = 'https://api-dev.bugzilla.mozilla.org/latest'

Is there a semi-stable API instead of latest? In the event of an update, I'd rather we have an API that works temporarily for users until we have a change to make it work against the latest.

>+# Patterns used on the patch filename to infer its relevant bug id.
>+PATCH_NAME_PATTERNS = [
>+  re.compile('bug-(?P<bug_id>[0-9]+)\.(diff|patch)')
>+  ]

This approach worries me - I don't name my patches after bug numbers (I give them more descriptive names) and users who have a non-default patchname pattern also lose here.

>+# Patterns used on the patch message, at the top of the patch file, to
>+# infer its relevant bug id.
>+MESSAGE_PATTERNS = [
>+  re.compile('[B|b]ug\s(?P<bug_id>[0-9]+)\s.*')
I'd also suggest just any number. Some people use b123456 as their pattern for example.

>+  ]
>+
>+# This is basically this module's interface to the internet. Change it
>+# for unit testing purposes.
>+build_opener = urllib2.build_opener
Hah, unit tests! Yeah, we need those.

>+def infer_bug_id(patchname, header_message=None):
>+  """Infer the bug id from the given patch name and header message list.
>+
>+  Examples:
>+
>+    >>> infer_bug_id('bug-535.diff')
>+    535
>+
>+    >>> infer_bug_id('foo.diff', ['Bug 535 - "do stuff" [r=adw]'])
>+    535
>+
>+    >>> infer_bug_id('blegh')
>+    Traceback (most recent call last):
>+    ...
>+    Abort: Cannot infer bug id from patch blegh.
>+  """
>+
>+  if header_message is None:
>+    header_message = []
>+
>+  for regexp in PATCH_NAME_PATTERNS:
>+    match = regexp.match(patchname)
>+    if match:
>+      return int(match.group('bug_id'))
>+
>+  for message in header_message:
>+    for regexp in MESSAGE_PATTERNS:
>+      match = regexp.match(message)
>+      if match:
>+        return int(match.group('bug_id'))
>+
>+  raise util.Abort("Cannot infer bug id from patch %s." % patchname)

We should prompt the user here. Ideally we could also fetch the bug title and confirm it with them.
>+    json_response = json.loads(response.read())
>+    if 'error' in json_response and json_response['error']:
>+      raise BugzillaApiError(repr(json_response))

Does this get pretty printed somewhere or does it kinda just barf on the terminal?
>+
>+def qexportbz(ui, repo, patch=None, **opts):
>+  """commit the current or given patch to its associated bugzilla bug."""
>+
>+  api_url = ui.config('qimportbz', 'bugzilla_rest_api_url',
>+                      DEFAULT_BZ_REST_API)
>+  username = opts['username'] or ui.config('qimportbz', 'username', '')
>+  password = opts['password'] or ui.config('qimportbz', 'password', '')
>+  if not username:
>+    raise util.Abort('Bugzilla username not specified.')

I think we might as well prompt the user here before complaining since it's done for passwords.

>+  header = mq.patchheader(patchfile)
>+  bug_id = infer_bug_id(patchname, header.message)
>+  contents = open(patchfile).read()
>+
>+  bzapi = BugzillaApi(ui=ui,
>+                      api_url=api_url,
>+                      username=username,
>+                      password=password)
>+
>+  ui.status('Obtaining information about existing patches for bug %d.\n' %
>+            bug_id)
>+
>+  attachments = bzapi.request('/bug/%d' % bug_id).get('attachments', [])
>+
>+  to_obsolete = [attachment
>+                 for attachment in attachments
>+                 if int(attachment['is_patch'])
>+                 and not int(attachment['is_obsolete'])]
>+
>+  if to_obsolete:
>+    ui.write('The following patches can be obsoleted:\n')
>+    for attachment in to_obsolete:
>+      ui.write('  attachment %7d - '
>+               '%s by %s\n' % (int(attachment['id']),
>+                               attachment['description'],
>+                               attachment['attacher']['name']))
>+    result = ui.prompt("Obsolete these patches (Y/n)?", default="Y")
>+    if result in ['N', 'n']:
>+      to_obsolete = []

Sometimes there are multi-part patches - can we have a selection UI here like in the qimport code? (in fact, this seems very similar to code over there).

>+  bzapi.request('/bug/%d/attachment' % bug_id, data=attachment)
>+
>+  for attachment in to_obsolete:
>+    attachment['is_obsolete'] = True
>+    a_id = int(attachment['id'])
>+    ui.status('Obsoleting attachment %d.\n' % a_id)
>+    bzapi.request('/attachment/%d' % a_id, data=attachment,
>+                  method='PUT')
>+
>+cmdtable = {
>+  '^qexportbz': (qexportbz, 
>+                [('d', 'description', 'patch', 'use <text> as description'),
>+                 ('u', 'username', '', 'your bugzilla username'),
>+                 ('p', 'password', '', 'your bugzilla password')],
>+                'hg qexportbz [options] [patch]')
>+}

I would actually like to see this be just qexport and have it take a bz url. I have a feeling that qexport to pastebin would be very popular and this also is symmetric with the qimport command.

Comment 6

[Atul Varma [:atul]]

Attachment: patch 2 - now with unit test suite and http logging

Comment 7

[Atul Varma [:atul]]

Ok, cool. I'm going ahead and writing a unit test suite now to ensure that there's no regression as I make the changes you're requesting.

Since there's some overlap in functionality between the import and export code, I should probably just go ahead and unit test both--right now this patch is just testing qexportbz though.

Comment 8

[Reed Loden [:reed]]

(In reply to comment #5)
> (From update of attachment 432492 [details] [diff] [review])
> >+DEFAULT_BZ_REST_API = 'https://api-dev.bugzilla.mozilla.org/latest'
> 
> Is there a semi-stable API instead of latest? In the event of an update, I'd
> rather we have an API that works temporarily for users until we have a change
> to make it work against the latest.

I'd much rather you use 'latest' here, especially considering BzAPI's recent security bug, as Gerv only fixed the bug in the newest rev (0.5) and didn't backport it to older revs.

Comment 9

[Atul Varma [:atul]]

(In reply to comment #5)
> (From update of attachment 432492 [details] [diff] [review])
> >+DEFAULT_BZ_REST_API = 'https://api-dev.bugzilla.mozilla.org/latest'
> 
> Is there a semi-stable API instead of latest? In the event of an update, I'd
> rather we have an API that works temporarily for users until we have a change
> to make it work against the latest.

Good idea; I guess I will replace this with the 0.5 api, as that is what /latest currently points to. 

> >+# Patterns used on the patch filename to infer its relevant bug id.
> >+PATCH_NAME_PATTERNS = [
> >+  re.compile('bug-(?P<bug_id>[0-9]+)\.(diff|patch)')
> >+  ]
> 
> This approach worries me - I don't name my patches after bug numbers (I give
> them more descriptive names) and users who have a non-default patchname pattern
> also lose here.

O. I thought qimportbz named them according to these inference rules by default when importing patches, so I was just following that convention. I would like it to infer as much as possible though, since being able to just type 'hg qexportbz' or 'hg qexport' and have the thingy take care of everything else for you is super easy.

> >+  raise util.Abort("Cannot infer bug id from patch %s." % patchname)
> 
> We should prompt the user here. Ideally we could also fetch the bug title and
> confirm it with them.

Yep, will add this.

> >+    json_response = json.loads(response.read())
> >+    if 'error' in json_response and json_response['error']:
> >+      raise BugzillaApiError(repr(json_response))
> 
> Does this get pretty printed somewhere or does it kinda just barf on the
> terminal?

Barf! It's a weird edge case; not even sure what situations it arises in.

> I think we might as well prompt the user here before complaining since it's
> done for passwords.

Okay.

> Sometimes there are multi-part patches - can we have a selection UI here like
> in the qimport code? (in fact, this seems very similar to code over there).

Sure, I will consolidate the code between qimport and qexportbz.

> I would actually like to see this be just qexport and have it take a bz url. I
> have a feeling that qexport to pastebin would be very popular and this also is
> symmetric with the qimport command.

Okay, will do.

Comment 10

[Atul Varma [:atul]]

Oops, my last comment mid-air collided w/ Reed's comment. I'll wait for further instructions re: what API version to use by default.

Comment 11

[Justin Dolske [:Dolske]]

(In reply to comment #2)

> * Not sure if it's a good idea or not to let people put their plaintext
> bugzilla password in their .hgrc.

Random thought: Due to security concerns, I wouldn't store my password on my untrusted build boxes, but nor can I in fact remember my rather long password to type it in when prompted. *sigh*

I suppose the long term solution would be to sprinkle some magic OAuth pixie dust into bugzilla...  But I wonder if a short-term solution would be to hack Bugzilla to allow unauthenticated attachments, and mark them with an "unauthenticated" flag which renders them invisible until the user logs in to Bugzilla and unchecks the flag. Seems like that could be simple, but I don't know Bugzilla. :)

Anyway, can't wait to try out qexportbz... somehow.

Comment 12

[Gervase Markham [:gerv]]

(In reply to comment #8)
> I'd much rather you use 'latest' here, especially considering BzAPI's recent
> security bug, as Gerv only fixed the bug in the newest rev (0.5) and didn't
> backport it to older revs.

Well, I disabled 0.1 to 0.3 entirely, and plan to disable 0.4 and 0.4.1 in the next few days, when people have had a chance to upgrade.

Certainly, using /latest is a way to avoid such problems, but at the possible risk of something breaking on you. However, the API is fairly stable now. But my plan is that it's not going to be 1.0 until the Bugzilla team reviews it.

Re: OAuth - the API can't fix this, Bugzilla needs to support it natively. 

However, do let me know if there are API improvements you think I can make to support your tools.

Gerv

Comment 13

[Atul Varma [:atul]]

Ugh, it should also be noted that I just tried using my patch on a Windows box with mozilla-build installed, and the simplejson dependency is annoying to satisfy, because mozilla-build's hg's Python is frozen. Might need to statically package simplejson along with qimportbz as qimportbz.simplejson here, or something weird.

Comment 14

[Rob Arnold [:robarnold]]

(In reply to comment #13)
> Ugh, it should also be noted that I just tried using my patch on a Windows box
> with mozilla-build installed, and the simplejson dependency is annoying to
> satisfy, because mozilla-build's hg's Python is frozen. Might need to
> statically package simplejson along with qimportbz as qimportbz.simplejson
> here, or something weird.

We could also update MozillaBuild to use a newer version of hg/python and require that (at some point I need to come up with a support policy).

I'm also wondering how feasible it would be to use the bugzilla XMLRPC interface instead of this ReST API since that would minimize any security/troubleshooting issues.

Comment 15

[Philipp Kewisch [:Fallen][☁️📆]]

Just in case someone wants to borrow ideas and/or copy code, I was working on something quite similar myself. I personally like the REST API and don't have a problem with proxying my password as long as its somewhere "trusted" like under *.bugzilla.mozilla.org.

My extension probably has some of the same issues as noted above (i.e bugzilla url), but it does contain some simple parsing using ui.edit() to allow posting a commit comment and setting review flags. I also have external dependencies (restkit and maybe even python 2.6).

The extension also allows automatically marking bugs as fixed (with a push comment) when using hg push.

http://hg.mozilla.org/users/mozilla_kewis.ch/hg-bugzilla/

Comment 16

[Atul Varma [:atul]]

Erm, so I'm now actually using pbranch instead of mq, and I'm using a simple tool I made to fetch a patch and output to stdout the patch with HG headers that contain info relating to the bug the patch is for. It also does the reverse, so I can use it via pipes through any kind of script.

My repo's here for anyone who's interested, though the major downside is that there's no docs yet:

  http://hg.toolness.com/pybugzilla/

But let me know if you're interested in this and I will write some up.

In any case, since I don't use mq anymore, I'm going to unassign myself from this bug... Feel free to take it up if you like, and feel free to throw out my patch and start a new one!

Comment 17

[(not currently active) Ted Mielczarek]

I didn't know this existed, but I basically wrote the same thing as a standalone extension:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/bzexport/

It's sneaky and borrows your Bugzilla login cookies from your Firefox profile, so you shouldn't have to provide your username and password. (That trick doesn't work on Windows for the same reasons comment 13 mentions.)

Comment 18

[Steve Fink [:sfink] [:s:]]

I'm going to make the courageous decision to close this bug, as ted's bzexport has served us well for the last 7 years. And there's a slight irony in closing this bug on the eve of bzexport being succeeded by arc (the phabricator uploader thing.)

Or maybe it is; I haven't tried arc yet.