1244602 - rewrite the bmo --> reviewboard connector to create a bug instead of updating reviewboard

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Extensions: MozReview Integration, defect)

Description

[:glob ✱]

the bmo --> reviewboard connector uses the admin credentials to log into reviewboard.

it looks like these credentials were changed on 2015-12-03 and the connector has been failing since.  there are 210 queued updates, which are generated when a bug is flipped from public to private.


the connector shouldn't use the rb admin account - it should have its own account (with admin rights if required).


i'll file another bug to get nagios alerts happening.

Comment 2

[:glob ✱]

for the time being i've disabled the reviewboard push connector.

as far as i can tell when the credentials are fixed the connect will delete the existing reviews in r/27117.
we probably don't want that to happen.

Comment 3

[Mark Côté [:mcote]]

Hm I thought this was actually disabled already.

The solution we came up with way back isn't complete because the commits will still live on in the Review Board hg repo.  It is, I believe, nontrivial to automatically strip these commits.  Perhaps we should just be triggering an email so a MozReview admin can do some cleanup.

Comment 4

[Gregory Szorc [:gps]]

There is a mechanism to strip commits from the review repo. See https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/reviewboard/tests/test-remote-strip.t#l95

Comment 5

[:glob ✱]

the outcomes of the discussions about this issue are:

(a) surprise that this was still enabled
(b) this is a problem we need to care about
(c) it's complicated to automate fixing
(d) we should notify someone when this rare event happens so it can be manually fixed


i'll redo the review board push connector to file a new bug in the "Developer Services::MozReview" component when a review needs to be updated.

notes to self:
- have the product and component as setting
- have a comma separated list of users to needinfo as a setting
- the bug should reference a mana page which describes the steps reqiured

Comment 6

[:glob ✱]

Attachment: 1244602_1.patch

i ended up not doing the needinfo part because everyone who needs to take action will be watching the component.

will add a link to mana once the page exists.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8717338 [details] [diff] [review]
1244602_1.patch

Review of attachment 8717338 [details] [diff] [review]:
-----------------------------------------------------------------

testing should be done shortly, code looks good.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8717338 [details] [diff] [review]
1244602_1.patch

Review of attachment 8717338 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

tested -- it works. However the version was hard-coded to Production. I suppose that's fine.
If Developer Services is ever renamed the configuration will break, and if the Production version isn't there it also breaks.
These are transient errors so I guess that is fine.

Will there be nagios monitoring of transient push connect errors?

Comment 9

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #8)
> tested -- it works. However the version was hard-coded to Production. I
> suppose that's fine.

good point; that's a trivial change that i'll fix on commit.

> Will there be nagios monitoring of transient push connect errors?

yes, i created the script in bug 1244604 and will hook it up once the process is in place.

Comment 10

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   9c75c64..4cdd5e5  master -> master

Comment 11

[:glob ✱]

this is now live on production.