Closed Bug 1033394 Opened 7 years ago Closed 6 years ago

Switch bzexport to using the native Bugzilla REST API

Categories

(Developer Services :: Mercurial: bzexport, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: emorley, Assigned: gps)

References

Details

(Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/789] )

Attachments

(27 files, 5 obsolete files)

1.19 KB, patch
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
emorley
: review+
sfink
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
emorley
: review+
sfink
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
sfink
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
sfink
: review+
emorley
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
emorley
: review+
smacleod
: review+
sfink
: review+
Details
bzexport currently uses:
https://api-dev.bugzilla.mozilla.org/latest/

The new endpoint, which "should" be identical is:
https://bugzilla.mozilla.org/bzapi/

Ted already landed the switch (as a no bug):
https://hg.mozilla.org/hgcustom/version-control-tools/rev/782f314c73d4

However this caused bug 1033258, so I had to revert in:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/30b6d858ed1c

We should figure out what needs fixing in the compatibility layer so that both bzexport and other projects can switch to it without other changes (if possible).
No longer blocks: 1033258
Depends on: 1033258
Summary: Switch bzexport to the new bzapi compatibility layer endpoint once the cause of bug 1033258 fixed → Switch bzexport to the new bzapi compatibility layer endpoint once bug 1033258 fixed
Depends on: 1033445
PSA: There is a new Python client API for the REST API: https://github.com/AutomatedTester/Bugsy. The package is checked into the version-control-tools repo. It still has a ways to go before it can support bzexport's needs. But I'd like to think bzexport's custom Bugzilla Python module is going the way of the dodo.
Sorry, guess I should have filed a bug to track that! Looks like we can re-land this.
glob says the production push that will pick up bug 1033258 should be in ~12 hours, so if nobody beats me to it I'll re-land tomorrow.
Assignee: nobody → ted
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> glob says the production push that will pick up bug 1033258 should be in ~12
> hours, so if nobody beats me to it I'll re-land tomorrow.

this is now live.
http://hg.mozilla.org/hgcustom/version-control-tools/rev/1c20d2343aee
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Reverted since review request comments are missing from bugmails when using the native APIs, whereas they are not when using the legacy bzapi (see bug 1054181 & bug 508541). This issue is also affecting reviewboard integration.

https://hg.mozilla.org/hgcustom/version-control-tools/rev/dfb611fc22be
Status: RESOLVED → REOPENED
Depends on: 508541
Resolution: FIXED → ---
Summary: Switch bzexport to the new bzapi compatibility layer endpoint once bug 1033258 fixed → Switch bzexport to the new bzapi compatibility layer endpoint once bug 508541 fixed
Summary: Switch bzexport to the new bzapi compatibility layer endpoint once bug 508541 fixed → Switch bzexport to the new bzapi compatibility layer endpoint
This was also causing the following when attaching new version of patches:
Error: There is no flag with the id '968857'.
abort: Could not update attachment 8483868 [details] [diff] [review]: HTTP Error 404:
Product: Other Applications → Developer Services
The blocker bugs here should be fixed now.  Can we try this again?
The version-control-tools repo now supports running BMO (via Docker) as part of tests. We can actually write reasonable tests for bzexport now. I would encourage this since bzexport's Mercurial extension currently has 0% test coverage.
Attached patch template for mercurial tests (obsolete) β€” β€” Splinter Review
Here is a template for a bzexport test using BMO running in Docker. The
test fails, possibly because the Dockerized BMO isn't exposing bzapi?
I'm not sure and I have other priorities to look at now (reviewboard).
Assignee: ted → gps
Status: REOPENED → ASSIGNED
I really wish bzexport had a field for "don't assign bug when uploading"
Assignee: gps → nobody
Status: ASSIGNED → NEW
(In reply to Gregory Szorc [:gps] from comment #11)
> I really wish bzexport had a field for "don't assign bug when uploading"

--no-take-bug iirc
Whiteboard: [kanban:engops:https://kanbanize.com/ctrl_board/6/227]
Whiteboard: [kanban:engops:https://kanbanize.com/ctrl_board/6/227] → [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/789] [kanban:engops:https://kanbanize.com/ctrl_board/6/227]
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/789] [kanban:engops:https://kanbanize.com/ctrl_board/6/227] → [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/789]
Assignee: nobody → emorley
Status: NEW → ASSIGNED
Attachment #8529009 - Attachment is obsolete: true
Attachment #8529012 - Attachment is obsolete: true
Blocks: 1098342
Depends on: 1105433
(In reply to Mike Hommey [:glandium] from comment #7)
> This was also causing the following when attaching new version of patches:
> Error: There is no flag with the id '968857'.
> abort: Could not update attachment 8483868 [details] [diff] [review]: HTTP
> Error 404:

I've tracked this down to bug 1105433.
We'll need to wait until that's fixed before trying to land this again.
Attachment #8503302 - Attachment is obsolete: true
I just added some .t tests for bzexport to version-control-tools. They run Bugzilla inside Docker. If we write enough tests, we'll uncover bugs in bzapi. I may very well go on a bzexport test-writing binge later today.
I'm about to make some people happy.
Depends on: 1119069
Attached file MozReview Request: bz://1033394/gps (obsolete) β€”
Attachment #8545777 - Flags: review?(sphink)
Attachment #8545777 - Flags: review?(smacleod)
Attachment #8545777 - Flags: review?(emorley)
/r/2129 - tests: dump more bug state
/r/2131 - bzexport: test that uploading a replacement patch works
/r/2133 - bzexport: add test for reviewer selection
/r/2135 - bzexport: partially implement reviewer switching test
/r/2137 - bzexport: enable loading modules from version-control-tools
/r/2139 - bzexport: use win_get_folder_path from mozhg.auth
/r/2141 - bzexport: use profile path finding from mozhg.auth
/r/2143 - bzexport: use profiles loading code from mozhg.auth
/r/2145 - bzexport: use mozhg.auth for finding cookies from profile
/r/2147 - bzexport: use mozhg.auth.getbugzillaauth
/r/2149 - bzexport: support for using a requests.Session for making requests
/r/2151 - bzexport: use REST API to obsolete attachments (bug 1033394)

Pull down these commits:

hg pull review -r f74d43e4dd47d040b80f8b2015c78eb515921e33
https://reviewboard.mozilla.org/r/2127/#review1371

A few comments on the state of the series.

First, I just noticed some local test failures due to expected bug status mismatch. Lots of `CONFIRMED` should be `UNCONFIRMED`.

Second, I should probably write more tests around bzexport auth.

Third, I should probably deprecate the `bzexport.username` config variables. I imagine some people have username but no password defined. When we switch to `mozhg.auth`, we'll look in `bugzilla.username` instead of `bzexport.username`. Upon finding nothing there, we'll prompt for it. Users will be like "wtf, I have the username defined." I like pro-actively notifying users of deprecation. Alternatively, we could teach `mozhg.auth` about pre-defined username values. But that would mean supporting `bzexport.username` indefinitely. I'd prefer we encourage people to switch to `bugzilla.username`, as that is used by the `reviewboard` extension as well and saves people from having multiple definitions.

FWIW, the `mozhg.auth` code is already extensively tested. We just need to review the interaction with bzexport.
Assignee: emorley → nobody
Status: ASSIGNED → NEW
https://reviewboard.mozilla.org/r/2147/#review1377

::: hgext/bzexport/bzauth.py
(Diff revision 1)
> -    if not username:
> -        username = ui.prompt("Enter username for %s:" % bugzilla, default='')
> -    if not password:
> -        password = ui.getpass("Enter password for %s: " % username)

The equivalent logic in getbugzillaauth() only prompts for the password if the username isn't set (since the conditionals are nested over there). ie: it's no longer possible to set just the username in the hgrc, and enter the password by prompt. 

If this is intentional, that's fine - just wanted to call it out in case not :-)
Comment on attachment 8545777 [details]
MozReview Request: bz://1033394/gps

Thank you for doing this :-)
Attachment #8545777 - Flags: review?(emorley) → feedback+
Assignee: nobody → gps
Status: NEW → ASSIGNED
Duplicate of this bug: 1085354
No longer depends on: 1105433
https://reviewboard.mozilla.org/r/2147/#review1389

> The equivalent logic in getbugzillaauth() only prompts for the password if the username isn't set (since the conditionals are nested over there). ie: it's no longer possible to set just the username in the hgrc, and enter the password by prompt. 
> 
> If this is intentional, that's fine - just wanted to call it out in case not :-)

That seems bad -- IMHO, saving passwords in config files, especially if they're unencrypted, is awful and should be heavily discouraged (yet still allowed). Having just the username in the config file is totally fine, on the other hand (even though it's crap from a usability POV to enter the password constantly.) So disallowing just-username is an encouragement to put a password in the config file.
https://reviewboard.mozilla.org/r/2145/#review1393

Er... this patch seems to remove the old code and import the new code with a new name, but I didn't see where it switched to it. Oh well, I will just assume this patch series converges.
https://reviewboard.mozilla.org/r/2145/#review1395

mozhg.auth has existed for a while. It was written as part of the MozReview work. The code is heavily based on bzexport.
https://reviewboard.mozilla.org/r/2143/#review1397

Hrm. This returns the first profile. Unless you're sorting them in a useful way, this is going to break my setup. Though the comments seem to indicate that this may be a temporary state, and eventually you'll search all profiles. I'll keep reading.
https://reviewboard.mozilla.org/r/2141/#review1399

It's a little weird to give r+ based on switching to mysterious code I haven't read, but I'm lazy and it's tested, so whatever.
https://reviewboard.mozilla.org/r/2139/#review1401

It's sad that the new code is identical, since it's ugly code. Oh well.
https://reviewboard.mozilla.org/r/2137/#review1403

Now running a mystery file ../bootstrap.py, after importing things. La de da.
https://reviewboard.mozilla.org/r/2135/#review1405

There's no way to mark a test as an expected failure?

(And am I now re-reviewing stuff edmorley has r+'d? RB is so confusing...)
https://reviewboard.mozilla.org/r/2129/#review1411

It might be nice to have options for what to include, but this doesn't seem too crazy yet.
My pass was more of a f+
Ok, I've reviewed a bunch of pieces here. I'm at a bit of a loss to figure out what else I should look at, or how far along these changes are. I didn't see anything that made me more comfortable about the profiles[0] thing, at least, so I know at least one of these isn't marked shipit yet.
(In reply to Steve Fink [:sfink, :s:] from comment #43)
> I didn't
> see anything that made me more comfortable about the profiles[0] thing, at
> least, so I know at least one of these isn't marked shipit yet.

https://reviewboard.mozilla.org/r/2147/ makes bzexport use:
https://hg.mozilla.org/hgcustom/version-control-tools/file/f7ba7ce1adda/pylib/mozhg/mozhg/auth.py#l62
(In reply to Ed Morley [:edmorley] from comment #44)
> (In reply to Steve Fink [:sfink, :s:] from comment #43)
> > I didn't
> > see anything that made me more comfortable about the profiles[0] thing, at
> > least, so I know at least one of these isn't marked shipit yet.
> 
> https://reviewboard.mozilla.org/r/2147/ makes bzexport use:
> https://hg.mozilla.org/hgcustom/version-control-tools/file/f7ba7ce1adda/
> pylib/mozhg/mozhg/auth.py#l62

Ah, so it does. Thanks.
Attachment #8545777 - Flags: review?(sphink) → review+
https://reviewboard.mozilla.org/r/2145/#review1427

Sorry. That wasn't my point, but it turns out I was blind. It does indeed switch to the new name in this very patch. Ignore me.
Attachment #8545777 - Flags: review?(sphink)
Attachment #8545777 - Flags: review?(emorley)
Attachment #8545777 - Flags: review+
Attachment #8545777 - Flags: feedback+
/r/2129 - tests: dump more bug state
/r/2261 - bugzilla: do not use Bugsy for fetching bugs
/r/2263 - testing: add cc, blocks, and depends_on to bug dump
/r/2131 - bzexport: test that uploading a replacement patch works
/r/2133 - bzexport: add test for reviewer selection
/r/2135 - bzexport: partially implement reviewer switching test
/r/2137 - bzexport: enable loading modules from version-control-tools
/r/2139 - bzexport: use win_get_folder_path from mozhg.auth
/r/2141 - bzexport: use profile path finding from mozhg.auth
/r/2143 - bzexport: use profiles loading code from mozhg.auth
/r/2145 - bzexport: use mozhg.auth for finding cookies from profile
/r/2147 - bzexport: use mozhg.auth.getbugzillaauth
/r/2149 - bzexport: support for using a requests.Session for making requests
/r/2151 - bzexport: use REST API to obsolete attachments (bug 1033394)
/r/2265 - bzexport: deprecate bzexport.username and bzexport.password
/r/2267 - bzexport: test for assigning a bug
/r/2269 - bzexport: test for CC list on newbug
/r/2271 - bzexport: test for setting bug dependencies with newbug
/r/2273 - bzexport: add an API for performing REST requests
/r/2275 - bzexport: use REST for creating bugs
/r/2277 - bzexport: add test for reviewing feedback
/r/2279 - bzexport: fetch attachments via REST API
/r/2281 - bzexport: use REST API for attachment creation
/r/2283 - bzexport: use REST API for getting and updating bugs
/r/2285 - bzexport: use REST to obtain information about a user
/r/2287 - bzexport: search for users using REST API

Pull down these commits:

hg pull review -r 1fae1cb2295b07593d9491adb5d746f7a01891eb
https://reviewboard.mozilla.org/r/2127/#review1431

With this latest patch series, I've killed all uses of bzAPI except `bzapi/configuration`. That one is slightly more non-trivial than the others because the API doesn't map nicely to what's in the REST API AFAICT. And, some semi-complicated caching code in bzexport doesn't hurt either.

I still need to write cookie tests for this. I wouldn't be surprised if cookie auth were somehow busted.

I'm feeling pretty good about the test thoroughness. If we roll this out and it breaks something, it should be relatively easy to code up a test and code a fix to what we know the local Bugzilla is willing to accept.
https://reviewboard.mozilla.org/r/2143/#review1435

There is a custom sort function. The default profile is always first. So, behavior is preserved.

One thing that may bite us is when mozhg.auth attempts to read *all* profiles and returns cookies from the first one it finds.

I am very receptive to ideas to improve the configurability of mozhg.auth to work in more use cases. As I said, the module is very well tested, so we just need to write the code to appease people.
https://reviewboard.mozilla.org/r/2265/#review1447

::: hgext/bzexport/__init__.py
(Diff revision 1)
> +            'use bugzilla.password instead)\n')

Perhaps we could add "Or alternatively use the cookie authentication options instead." to promote moving away from password. (I'd suggest many people don't realise cookie support now exists).
Comment on attachment 8545777 [details]
MozReview Request: bz://1033394/gps

Take this as somewhere between an f+ and an r+, it's probably best if the others take a look too. Thank you for all the cleanup! :-)
Attachment #8545777 - Flags: review?(emorley) → review+
https://reviewboard.mozilla.org/r/2279/#review1603

::: hgext/bzexport/__init__.py
(Diff revision 1)
>      try:
> -        bug = json.load(urlopen(ui, req))
> -    except Exception, e:
> -        raise util.Abort(_("Could not load info for bug %s: %s") % (bug, str(e)))
> +        bugs = bz.get_attachments(auth, bugid)
> +    except Exception as e:
> +        raise util.Abort(e.message)
> +
> +    attachments = bugs['bugs'][bugid]

Huh?
bug = get_attachments()?
attachments = bugs['bugs'][bugid]?

Assuming this code is correct, then maybe

bug_attachments = get_attachments()
attachments = bug_attachments['bugs'][bugid]
https://reviewboard.mozilla.org/r/2127/#review1599

::: hgext/bzexport/__init__.py
(Diff revision 2)
> +    aid = result['attachments'].keys()[0]

I'd prefer a longer name than 'aid'. Took me a second to figure out it was attachment_id. attachment_id, attach_id, att_id would all be fine.
(In reply to Ed Morley [:edmorley] from comment #62)
> Comment on attachment 8545777 [details]
> MozReview Request: bz://1033394/gps
> 
> Take this as somewhere between an f+ and an r+, it's probably best if the
> others take a look too. Thank you for all the cleanup! :-)

Personally, I think edmorley counts as an r+, there's nothing my eyeballs can really add above his, other than just being an additional set.

(And can I request that RB *not* post individual Ship It! comments? Buffer them up and post them all as one comment, maybe, or make the status more visible in RB and don't bother posting them at all. Even for people who want bugzilla to track everything, I don't think it helps.)
Comment on attachment 8545777 [details]
MozReview Request: bz://1033394/gps

I think I looked at everything of interest (so far? I don't know if there's more that'll happen here.)
Attachment #8545777 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink, :s:] from comment #73)
> (And can I request that RB *not* post individual Ship It! comments? Buffer
> them up and post them all as one comment, maybe, or make the status more
> visible in RB and don't bother posting them at all. Even for people who want
> bugzilla to track everything, I don't think it helps.)

Yeah, we are almost certainly going to reduce the amount of mirroring from Review Board to Bugzilla and make the former the source of truth for anything reviewed by MozReview.  Furthermore, anything mirrored will probably have bugmail suppressed in favour of Review Board emails (although admittedly we might have to tweak those to avoid the same spam coming from a different source).
Attachment #8545777 - Flags: review+
https://reviewboard.mozilla.org/r/2265/#review1633

> Perhaps we could add "Or alternatively use the cookie authentication options instead." to promote moving away from password. (I'd suggest many people don't realise cookie support now exists).

I'll amend the message before landing to say something like "or log into Bugzilla using Firefox".
Comment on attachment 8545777 [details]
MozReview Request: bz://1033394/gps

I know you just landed this, but I just finished looking over the parts you asked r? to me. LGTM.
Attachment #8545777 - Flags: review?(smacleod) → review+
Everything I've written has been reviewed and landed.

As mentioned in comment 50, we now use REST for everything except bzapi/configuration. I'll file a follow-up to track that.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Blocks: 1121247
Blocks: 1121254
Summary: Switch bzexport to the new bzapi compatibility layer endpoint → Switch bzexport to using the native Bugzilla REST API
Duplicate of this bug: 1135625
Attachment #8545777 - Attachment is obsolete: true
Attachment #8618186 - Flags: review+
Attachment #8618187 - Flags: review+
Attachment #8618188 - Flags: review+
Attachment #8618189 - Flags: review+
Attachment #8618190 - Flags: review+
Attachment #8618191 - Flags: review+
Attachment #8618192 - Flags: review+
Attachment #8618193 - Flags: review+
Attachment #8618194 - Flags: review+
Attachment #8618195 - Flags: review+
Attachment #8618196 - Flags: review+
Attachment #8618197 - Flags: review+
Attachment #8618198 - Flags: review+
Attachment #8618199 - Flags: review+
Attachment #8618200 - Flags: review+
Attachment #8618201 - Flags: review+
Attachment #8618202 - Flags: review+
Attachment #8618203 - Flags: review+
Attachment #8618204 - Flags: review+
Attachment #8618205 - Flags: review+
Attachment #8618206 - Flags: review+
Attachment #8618207 - Flags: review+
Attachment #8618208 - Flags: review+
Attachment #8618209 - Flags: review+
Attachment #8618210 - Flags: review+
Attachment #8618211 - Flags: review+
You need to log in before you can comment on or make changes to this bug.