924405 - Replace bz.js/bzAPI usage with native Bugzilla REST API

Status: RESOLVED FIXED

(Tree Management :: Bugherder, defect)

Description

[Graeme McCutcheon [:graememcc]]

Switching to the native Bugzilla REST API will provide a number of benefits: one can find the exact email for the default assignee (which allows us to fix bug 909822) and exact default milestone information for all products (mcMerge currently guesses this).

Ideally, the replacement would offer the same API as bz.js to avoid code churn. For reference, at time of writing, mcMerge currently makes searchBugs, getBug, getConfiguraton and updateBug calls.

This is blocked on deployment of Bugzilla 4.4 to bmo. Prior versions do not supply the flag_type data for the components returned by the "products" method. mcMerge needs this to set in-testsuite? (and disallow setting it where appropriate).

Additionally, I would need access to a public Bugzilla 4.4+ install with the REST API enabled that I can configure for manual testing. Currently remap=1 in the URL provides an interface to redirect the bug updates to specified bugs in https://landfill.bugzilla.org/bzapi_sandbox - I administer the mcMerge test component on that installation and ensure the milestones mirror typical milestones on bmo. As far as I can tell, the REST API is not available there (neither rest.cgi or /rest seem to work).

Comment 1

[Mark Côté [:mcote]]

We have backported all native REST API work to bmo's current version, as we often do with needed features.  Everything that has been committed upstream is on bmo, and much of the work in progress is on bugzilla-dev.allizom.org.  I don't think this bug should block on bug 880965, which will also be backported when complete.

Landfill, however, probably will need to be upgraded, as it is not directly managed by the bmo team.  Depending on what you need to test, you can probably use bmo or bugzilla-dev.a.o.

Comment 2

[Mark Côté [:mcote]]

Sorry, I should say, the bzAPI compatibility layer (bug 880669) will also be backported, if you care about that.  Although I guess you are using new features, so it may not matter to you.

Upgrading is generally just to grab noncritical fixes and features.  We'll probably upgrade sometime early next year.

Comment 3

[Graeme McCutcheon [:graememcc]]

Ah. Looking at http://bzr.mozilla.org/bmo/4.2/view/head:/Bugzilla/WebService/Product.pm, it looks like the flag_type REST API changes (bug 741722, with a regression fix in bug 838846) haven't been backported to bmo yet.

Comment 4

[Mark Côté [:mcote]]

Ah entirely possible, if it wasn't needed for other things.  I will file a bug to backport them.  Won't take long.

Comment 5

[Ed Morley [:emorley]]

David, mcMerge uses bz.js (as do quite a few other projects) - do you think it's worth getting it updated upstream to use the new API?
https://github.com/harthur/bz.js

Comment 6

[David Lawrence [:dkl]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #5)
> David, mcMerge uses bz.js (as do quite a few other projects) - do you think
> it's worth getting it updated upstream to use the new API?
> https://github.com/harthur/bz.js

I do a fork and then a pull request with any needed changes. The main thing to remember is that the bz.js
lib is a small wrapper around the API calls and that the client code is what dictates what parameters are
passed and how it interprets the return data. So I can see what changes can be made but some work will
still need to be done on the client side in some cases too.

dkl

Comment 7

[David Lawrence [:dkl]]

(In reply to Graeme McCutcheon [:graememcc] from comment #3)
> Ah. Looking at
> http://bzr.mozilla.org/bmo/4.2/view/head:/Bugzilla/WebService/Product.pm, it
> looks like the flag_type REST API changes (bug 741722, with a regression fix
> in bug 838846) haven't been backported to bmo yet.

bug 741722 is in production. The flag_types are included with each component so load time is not great:
https://bugzilla.mozilla.org/rest/product/Bugzilla

Also bug 838846 fix is in production:
https://bugzilla.mozilla.org/rest/product/Bugzilla?include_fields=components&exclude_fields=components.flag_types

dkl

Comment 8

[Ed Morley [:emorley]]

I can see changes landing on the bz.js 1.0.x branch (https://github.com/harthur/bz.js/tree/1.0.x) for REST compatibility - I'm presuming these are still a WIP and so holding off looking at this any more for now.

Comment 9

[Ed Morley [:emorley]]

Bug 1027031 makes this slightly less urgent, since we can switch to bmo without having to immediately switch to the native API and fix the calls accordingly.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Link to mcMerge PR #5

I used this locally for a merge from fx-team to m-c just now. The merge worked (all of the bugs were commented and the relevant bugs were resolved, and the milestones were set as I selected them.

The relevant changes I made:
1) Move the bzAPI urls to a different variable for use as needed (I couldn't find an equivalent to bzAPI's /configuration call, so I'm still calling bzAPI for that), while it uses the REST api's URL for everything else.
2) bzAPI's "username" authentication parameter is named "login" in the native API.


I'd welcome some more testing from people, though.

Comment 11

[David Lawrence [:dkl]]

(In reply to Wes Kocher (:KWierso) from comment #10)
> 1) Move the bzAPI urls to a different variable for use as needed (I couldn't
> find an equivalent to bzAPI's /configuration call, so I'm still calling
> bzAPI for that), while it uses the REST api's URL for everything else.

Sorry not more obvious. I will file and upstream bug to make /rest/configuration (native) work same as /bzapi/configuration.
Until then, this does the exact same as /bzapi/configuration:

https://bugzilla.mozilla.org/config.cgi?ctype=json

> 2) bzAPI's "username" authentication parameter is named "login" in the
> native API.

Yeah username only works with the /bzapi path. FWIW, you can also use x-bugzilla-login and x-bugzilla-password in the headers now instead.

dkl

Comment 12

[Ed Morley [:emorley]]

Comment on attachment 8585743 [details] [review]
Link to mcMerge PR #5

Ah missed this - the needinfo flag on me got cleared when dkl commented.
Adding review? now I'm back :-)

Comment 13

[Ed Morley [:emorley]]

Comment on attachment 8585743 [details] [review]
Link to mcMerge PR #5

I'm happy rubber stamping this for now - though longer term I think it would make sense to update bz.js upstream:
https://github.com/canuckistani/bz.js

:-)

Comment 14

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #13)
> I'm happy rubber stamping this for now - though longer term I think it would
> make sense to update bz.js upstream:
> https://github.com/canuckistani/bz.js

Master is still comparable with 0.3.x, the new rest api changes are currently only on the 1.0.x branch (it's probably time to fix it up and merge to master):
https://github.com/canuckistani/bz.js/compare/0.3x...1.0.x

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I went ahead and landed this in https://github.com/mozilla/mcMerge/commit/6b31c14aaa7497c5d95a0fcbd9eb95ff9b508a89 as is to see if it'll fix some issues we were having with McMerge hitting bzAPI today. (Native REST seemed to work, bzAPI didn't)

I'll file followups to clean things up a bit in the future when that /configuration change happens and maybe change the login stuff from parameters to headers.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Reverted because it seems comments don't get submitted during bug marking:
https://github.com/mozilla/mcMerge/commit/37a8814542a3e9459bd313e2251d7dace7bedca8

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Link to pull request #8

I merged pull request 5, so I guess that means I can't use it any more for further work. Here's pull request #8, which gets me a little closer to being able to land this for real.

Testing locally, it now resolves the bugs, removes the checkin-needed keyword if it was set, and creates the comments with the revision included.

This still uses the legacy URL for the /configuration call (I was getting weird results when I tried using https://bugzilla.mozilla.org/config.cgi?ctype=json and didn't feel like digging into that today).

I'm using the x-bugzilla-login/password headers for the login information for the native REST calls.

I'd say we're good to go, but the Bugherder UI is acting weird for me. After submitting the changes, the "Submit" button is still active, and if I go to the Summary page, it acts like I haven't submitted anything.



Putting this up for feedback and for any ideas why the UI's acting like that.

Comment 18

[Ed Morley [:emorley]]

Comment on attachment 8595048 [details] [review]
Link to pull request #8

Since more substantial changes are required than in the previous version of the patch, I think it's best if Bugherder is switched to use the 1.x branch of bz.js per comment 14, rather than forking it/duplicating James Lal's changes in the 1.x branch to add native REST support. That may even fix some of the issues with the current PR :-)

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Ed Morley [:emorley] from comment #18)
> Comment on attachment 8595048 [details] [review]
> Link to pull request #8
> 
> Since more substantial changes are required than in the previous version of
> the patch, I think it's best if Bugherder is switched to use the 1.x branch
> of bz.js per comment 14, rather than forking it/duplicating James Lal's
> changes in the 1.x branch to add native REST support. That may even fix some
> of the issues with the current PR :-)

Ugh, there's no browser version of the 1.0.x branch available, and I can't seem to build one myself despite the "just use 'browserbuild bz.js'" in the README file.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

[15:39]	KWierso	lightsofapollo|pto: okay, I'll ping again tomorrow
[15:39]	lightsofapollo|pto	KWierso: auswerk, garndt or jonasfj are good names to ping... if I am a blocker somewhere that is a bug[lemme know if this is the case]
[15:40]	KWierso	lightsofapollo|pto: it's about bz.js's 1.0.x branch, which you're the last one to touch
[15:40]	lightsofapollo|pto	KWierso: oh wow long time ago!
[15:40]	lightsofapollo|pto	KWierso: we don't use that for anything at this point... it had some upgrades/etc.. but super out of date now probably
[15:41]	KWierso	lightsofapollo|pto: was going to attempt to upgrade mcMerge (now Bugherder) from 0.3 to 1.0.x to pick up the native REST changes
[15:41]	lightsofapollo|pto	KWierso: ah gotcha... well the 1.x branch does start that but it only added what I needed at the time
[15:42]	lightsofapollo|pto	the lib is unsupported now iirc
[15:42]	KWierso	lightsofapollo|pto: yeah, I've been tweaking mcMerge's copy of the 0.3 file to get what I needed. emorley|away thought upgrading it to the 1.0.x branch would pick it all up more easily
[15:42]	KWierso	I'll comment in the bug, thanks
[15:43]	lightsofapollo|pto	ouch

Comment 21

[Ed Morley [:emorley]]

Oh, that's not what he implied in bug 974363 comment 7 (just noticed this bug was previously marked as depending on that bug).

In which case it's up to you how you want to proceed :-)

If it were me at this point, I would almost be inclined to WONTFIX this bug, seeing as the /bzapi/ endpoint is here for the long haul (if I've understood correctly).

Comment 22

[Ed Morley [:emorley]]

Oh though I guess we'd need the new API for bug 975072?
meh.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This will also make bug 909822 easier, if I understood things right, since native REST gives full email addresses that would be easier to match against.

Comment 24

[Ed Morley [:emorley]]

Ah good timing: https://github.com/canuckistani/bz.js/issues/26 ! :-)

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to David Lawrence [:dkl] from comment #11)
> Sorry not more obvious. I will file and upstream bug to make
> /rest/configuration (native) work same as /bzapi/configuration.
> Until then, this does the exact same as /bzapi/configuration:
> 
> https://bugzilla.mozilla.org/config.cgi?ctype=json
I'm getting the request to this rejected by CORS. :(

Comment 26

[David Lawrence [:dkl]]

(In reply to Wes Kocher (:KWierso) from comment #25)
> (In reply to David Lawrence [:dkl] from comment #11)
> > Sorry not more obvious. I will file and upstream bug to make
> > /rest/configuration (native) work same as /bzapi/configuration.
> > Until then, this does the exact same as /bzapi/configuration:
> > 
> > https://bugzilla.mozilla.org/config.cgi?ctype=json
> I'm getting the request to this rejected by CORS. :(

Once bug 504937 is landed upstream, I will backport it to BMO soon after and so that would be better suited for this. I am reluctant to add CORS headers to *.cgi scripts so I would rather you use that when it ready. Until than is it possible to continue to use /bzapi/configuration just for that call and change later? Unfortunately the native version that lands will have a different data format that what /bzapi/configuration returns so there will be one time cost to convert.

dkl

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/b2g-inbound/rev/5d6a69634894

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/b2g-inbound/rev/5d6a69634894

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/b2g-inbound/rev/5d6a69634894

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/b2g-inbound/rev/5d6a69634894

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Sorry about that. Forgot to divert to landfill when I was testing.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: PR 30

This gets us closer, and uses the upstream bz.js. 

This PR should NOT land as-is because it would break diverting to landfill, as the /configuration request is hardcoded to official bmo's configuration within bz.js, which won't match landfill's configuration, which will probably stop anything from working properly.

I'll file an upstream issue with bz.js to see if we could get that a bit more flexible. Might end up just patching it in bugherder instead. Dunno.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Used PR 30 to mark a 50-commit merge to mozilla-central. Everything appears to have worked.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And it looks like the status_firefox43 flags and target milestones are being correctly set.

Comment 35

[Ed Morley [:emorley]]

Comment on attachment 8647610 [details] [review]
PR 30

(looks ok at a very quick skim read :-))

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This needs something from around https://github.com/mozilla/bugherder/pull/31/files#diff-0f86ffb527a5b3661bf43a87273591c5L110 and from https://github.com/mozilla/bugherder/pull/31/files#diff-66fb557564720965af37d3c611845cedL178 in order to not break setting assignees. (First change because REST API returns full email addresses, not everything before '@'. Second change because REST API takes assignee setting in the form of an email address, not as an object with a 'name' property set to an email address.)

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/6bb8ca8a857c

Comment 38

[Graeme McCutcheon [:graememcc]]

Comment on attachment 8647610 [details] [review]
PR 30

LGTM.

To be fair, landfill redirection must be completely broken in any case: landfill.mozilla.org/bzapi_sandbox seems to be gone. I had a "mcMerge test product" product on that server with milestones etc mirroring bmo; the redirection process thus had to set any redirected bug's product to that product. This product won't exist on bz-dev.allizom.org. We'll need to rethink the whole diverting thing, though not necessarily in this bug.

In terms of /configuration discrepancies between bmo and bz-dev, as I understand it, bz-dev usually has a sanitized snapshot of the bmo database at some earlier point in time, so flag id's will match, but more recent flags and milestones might not be present.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8647610 [details] [review]
PR 30

Okay, I think we're good for an actual review request now. 

I folded in the things that seem to be necessary for getting redirects to bz-dev working correctly (or at least partially). If we need more than that, lets follow up in another bug.

This should also keep assignee setting working at least as good as the old system that bzapi had (though I guess now it won't catch '[e-mail]'). We'll do better with bug 909822.

Comment 40

[Graeme McCutcheon [:graememcc]]

Comment on attachment 8647610 [details] [review]
PR 30

> though I guess now it won't catch '[e-mail]'

To avoid regressing the NSS default assignee case until bug 909822 lands, we could maybe change the relevant line to:

bug.isUnassigned = /^nobody@(?:mozilla.org|nss.bugs)$/.test(bugObj.assigned_to.name);

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Landed with that change: https://github.com/mozilla/bugherder/commit/16bbe25c6823d464a63f46fc1a164e85a1dc9e47

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Reverted in https://github.com/mozilla/bugherder/commit/91490755206f510d1855d6c2a5689cbaccc736df because weird things are happening with commenting in bugs when marking merges.

Comment 43

[Ed Morley [:emorley]]

After chatting on IRC, it turns out no expires header is set at the moment:
https://emorley.pastebin.mozilla.org/8845724

And for Firefox at least, if we don't set an "Expires" or "Cache-control: max-age=N" header, the browser doesn't even bother checking to see if the ETag/Last-Modified has changed, for an amount of time equal to 10% of the time since the file last changed.

As such I asked Wes to modify the nginx config used by our buildpack (which we've already had to fork for other fixes), which he kindly did here:
https://github.com/edmorley/staticfile-buildpack/pull/1

I've now redeployed bugherder using the latest buildpack change:

[~/src/bugherder]$ stackato delete
Deleting application [bugherder] ... OK
[~/src/bugherder]$ stackato push --no-prompt
Pushing application 'bugherder'...
Framework:       buildpack
Runtime:         <framework-specific default>
Application Url: bugherder.paas.allizom.org
Creating Application [bugherder] ... OK
  Adding Environment Variable [BUILDPACK_URL=git://github.com/edmorley/staticfile-buildpack.git]
  Adding Environment Variable [FORCE_HTTPS=true]
Updating environment: OK
Uploading Application [bugherder] ...
  Checking for bad links:  OK
  Copying to temp space:  OK
  Checking for available resources:  OK
  Packing application: OK
  Uploading (1K):  OK
Push Status: OK
stackato.stager: Staging application 'bugherder'
staging:
staging: -----> git clone --depth 1 https://github.com/mozilla/bugherder.git src
staging:        Cloning into 'src'...
staging: -----> Stackato receiving staging request
staging: -----> Static file app detected
staging: -----> Root folder src
staging: -----> Copying project files into public/
staging: -----> Setting up nginx
staging: -----> Discovering process types
staging:        Procfile declares types -> web
staging: -----> rm -rf public/.git
staging: -----> echo "Cron not yet run on this instance!" > public/deploy.txt
staging: end of staging
stackato.stager: Completed staging application 'bugherder'
http://bugherder.paas.allizom.org/ deployed

And the headers are now being set:

[~/src/bugherder]$ curl -sI https://bugherder.paas.allizom.org/ | grep expires
expires: Wed, 09 Sep 2015 17:44:10 GMT

Note: Anyone who downloaded assets from the site _before_ the expires header was set, will still need to either force-reload, clear their cache, or wait for the 10% of time since last change to pass. After that, the assets will always expire 1 hr after they last accessed them.

Comment 44

[Ed Morley [:emorley]]

I've also redeployed the mcmerge.paas.allizom.org instance, since we have that running too:

[~/src/bugherder]$ curl -sI https://mcmerge.paas.allizom.org/js/BugData.js | grep expires
expires: Wed, 09 Sep 2015 17:59:28 GMT

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I went ahead and re-landed this in https://github.com/mozilla/bugherder/commit/7699353ad0ef18584688a01ee89fadc7a0eb2781 to see if comment 43's changes help for this.

Also landed https://github.com/mozilla/bugherder/commit/1c7dadb4e88d6dae7ca12edf5e426c8cd226431f because I noticed that bz.js was sending both "login" and "username" parameters (in the same request) to bugzilla, when the native REST API only uses "login".

Lets hope it sticks this time.