Closed Bug 1229468 Opened 9 years ago Closed 9 years ago

Review Board Web API to submit a series of review requests

Categories

(MozReview Graveyard :: General, defect)

Product:
MozReview Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(15 files)

MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor
40 bytes, text/x-review-board-request
dminor
: review+
smacleod
: review+
Details
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor
40 bytes, text/x-review-board-request
smacleod
: review+
dminor
: review+
Details
MozReview Request: mozreview: print more diff info; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor
58 bytes, text/x-review-board-request
dminor
: review+
smacleod
: review+
Details
MozReview Request: mozreview: log user events; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: reviewboard: add test for pushing to a discarded series; r?smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: reviewboard: use demandimport.deactivated; r?smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
Submitting review requests from Mercurial via the Review Board web API requires a gazillion HTTP requests. Series involving several commits take a long time to submit. Actions are performed non-atomically and the risk of failure partially through submission is higher than it should be. As we keep adding more and more actions to submission (such as LDAP checking), submitting review requests keeps getting slower. Let's establish a single Review Board Web API to ingest a series of commits and turn them into a review request series. This will be part of a larger effort to overhaul how commits are submitted to MozReview. This will enable the Git integration to be simpler.
mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod Currently, the Mercurial server makes several HTTP requests to Review Board's Web API to turn the pushed commits into review requests. Series with many commits take longer to submit than series with a single commit because we make additional requests for each review request / commit. This commit introduces a new Review Board Web API that aims to eventually replace the several existing Web API requests with a unified / single API call. Right now, this new API simply creates/updates the parent/squashed review request and diff. Eventually, more of the logic from pushhooks.py will move into the new Web API. I'm not an expert in the Review Board API. So there should be a thorough code review, which likely involves reading the new code next to existing code in Review Board's Web API.
Attachment #8694470 - Flags: review?(smacleod)
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/1-2/
mozreview: copy more read-only processing code; r?smacleod This is basically copying a lot of code from pushhooks.py. We don't yet make any database modification as part of this code. This just proves that a rough 1:1 copy continues to work.
Attachment #8694493 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/26757/#review24543 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:175 (Diff revision 2) > + squashed_draft = ReviewRequestDraft.create(squashed_rr) I think this is buggy. ReviewBoard doesn't copy fields from rr to draft. Instead, the Web API has the code for this. I fix this up as part of a not-yet-published commit. I may have to fold all these commits before final landing :/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/2-3/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/1-2/
Comment on attachment 8699069 [details] MozReview Request: mozreview: print more diff info; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/1-2/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/3-4/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/2-3/
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/1-2/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/4-5/
Attachment #8694470 - Attachment description: MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod → MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor
Attachment #8694470 - Flags: review?(dminor)
Attachment #8694493 - Attachment description: MozReview Request: mozreview: copy more read-only processing code; r?smacleod → MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r?smacleod, dminor
Attachment #8694493 - Flags: review?(dminor)
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/3-4/
Attachment #8699070 - Attachment description: MozReview Request: mozreview: create child review requests from batch API → MozReview Request: mozreview: create child review requests from batch API; r?smacleod, dminor
Attachment #8699070 - Flags: review?(smacleod)
Attachment #8699070 - Flags: review?(dminor)
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/2-3/
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/3-4/
Attachment #8699070 - Attachment description: MozReview Request: mozreview: create child review requests from batch API; r?smacleod, dminor → MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/4-5/
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/1-2/
Comment on attachment 8699069 [details] MozReview Request: mozreview: print more diff info; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/2-3/
Comment on attachment 8699565 [details] MozReview Request: mozreview: log user events; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/1-2/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/5-6/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/4-5/
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/5-6/
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/2-3/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor https://reviewboard.mozilla.org/r/26757/#review25415 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:37 (Diff revision 6) > + nit: please add a second empty line ::: pylib/mozreview/mozreview/resources/batch_review_request.py:57 (Diff revision 6) > + 'description': 'Repository to submit reviews to', nit: Repository to which to submit reviews ::: pylib/mozreview/mozreview/resources/batch_review_request.py:128 (Diff revision 6) > + identifier, local_site, False) nit: fix indentation ::: pylib/mozreview/mozreview/resources/batch_review_request.py:194 (Diff revision 6) > + Callers must call ``diffset.save()`` afterwards for changes to be preserved. Is there a use case for calling this function and then not saving the changes?
Attachment #8694470 - Flags: review?(dminor) → review+
Attachment #8694493 - Flags: review?(dminor) → review+
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor https://reviewboard.mozilla.org/r/26783/#review25425 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:101 (Diff revision 5) > + logger.error('commit missing key %s' % key) Please include the commit sha1 to make debugging easier. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:104 (Diff revision 5) > + 'commits': 'Individual commit missing key %s' % key}} Please include the commit sha1 to make debugging easier. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:273 (Diff revision 5) > + nit: remove extra newline
Attachment #8699070 - Flags: review?(dminor)
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor https://reviewboard.mozilla.org/r/28177/#review25391 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:236 (Diff revision 6) > + 'warnings': warnings, nit: please place in alphabetical order ::: pylib/mozreview/mozreview/resources/batch_review_request.py:509 (Diff revision 6) > + # TODO update reviewers on squashed draft. So I think the only reason we were setting reviewers on squashed review requests so they could be published Since we now allow publishing requests without reviewers we might be able to skip doing this work. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:536 (Diff revision 6) > +def previous_reviewers(rr): Since you have to rewrite this anyway, you might as well use the helper function here: https://dxr.mozilla.org/hgcustom_version-control-tools/source/pylib/mozreview/mozreview/review_helpers.py#10
Comment on attachment 8699773 [details] MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/1-2/
Attachment #8699773 - Attachment description: MozReview Request: reviewboard: INCOMPLETE use privileged RBTools client when setting LDAP association; r?smacleod → MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r?smacleod
Comment on attachment 8699069 [details] MozReview Request: mozreview: print more diff info; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/3-4/
Comment on attachment 8699565 [details] MozReview Request: mozreview: log user events; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/2-3/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/6-7/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/5-6/
Attachment #8699070 - Flags: review?(dminor)
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/6-7/
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/3-4/
Comment on attachment 8699769 [details] MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/1-2/
Comment on attachment 8699770 [details] MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/1-2/
Comment on attachment 8699771 [details] MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/1-2/
Comment on attachment 8699772 [details] MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/1-2/
Comment on attachment 8699774 [details] MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/1-2/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/7-8/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/6-7/
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/7-8/
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/4-5/
Comment on attachment 8699769 [details] MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/2-3/
Comment on attachment 8699770 [details] MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/2-3/
Comment on attachment 8699771 [details] MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/2-3/
Comment on attachment 8699772 [details] MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/2-3/
Comment on attachment 8699774 [details] MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/2-3/
https://reviewboard.mozilla.org/r/26755/#review26493 I've still got a single failure in test-auth.t in this series. But I think it's at the point where it can receive some serious review love. Hopefully the interdiffs will be small.
Attachment #8699070 - Flags: review?(dminor)
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor https://reviewboard.mozilla.org/r/28177/#review26543 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:658 (Diff revision 8) > + logger.info('recognized reviewer: %s' % reviewer) unrecognized reviewer?
Comment on attachment 8699773 [details] MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/2-3/
Comment on attachment 8699069 [details] MozReview Request: mozreview: print more diff info; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/4-5/
Comment on attachment 8699565 [details] MozReview Request: mozreview: log user events; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/3-4/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/8-9/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/7-8/
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/8-9/
Attachment #8699070 - Flags: review?(dminor)
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/5-6/
Comment on attachment 8699769 [details] MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/3-4/
Comment on attachment 8699770 [details] MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/3-4/
Comment on attachment 8699771 [details] MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/3-4/
Comment on attachment 8699772 [details] MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/3-4/
Comment on attachment 8699774 [details] MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/3-4/
Attachment #8699773 - Flags: review?(smacleod) → review+
Comment on attachment 8699773 [details] MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod https://reviewboard.mozilla.org/r/28445/#review26729 ::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:525 (Diff revision 3) > - password=privileged_password): > + password=privileged_password) as rbc: So strange... I've added some more testing around the security of our ldap association stuff just to be safe, it's in your review queue.
Attachment #8699069 - Flags: review?(smacleod) → review+
Comment on attachment 8699069 [details] MozReview Request: mozreview: print more diff info; r=smacleod https://reviewboard.mozilla.org/r/27509/#review26781
Comment on attachment 8699565 [details] MozReview Request: mozreview: log user events; r=smacleod https://reviewboard.mozilla.org/r/28277/#review26783
Attachment #8699565 - Flags: review?(smacleod) → review+
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor https://reviewboard.mozilla.org/r/26757/#review27041 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:74 (Diff revision 9) > + logger.info('processing post_series request for %s' % user) We call this "BatchReviewRequestResource" but refer to it as a "post_series" request here, we should be consistent with the naming. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:102 (Diff revision 9) > + repo = Repository.objects.get(pk=repo_id) You should be passing the `local_site` here as well. It's not strictly needed in our case but if we ever start taking advantage of local sites we might as well have things working (especially since you've already fetched it above). Local sites in Review Board are a way to host multiple Review Board's from the same database and web head (at separate paths I believe). When writing new stuff in the extensions it's probably a good idea to sprinkle the local site stuff around where Review Board does it itself. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:102 (Diff revision 9) > + repo = Repository.objects.get(pk=repo_id) > + except ObjectDoesNotExist: We might as well call `int()` on the id and catch a `ValueErorr` to return `INVALID_FORM_DATA` as well since we're only supporting the integer repo id here (On the Review Request endpoint you can use things like the repos name as well) ::: pylib/mozreview/mozreview/resources/batch_review_request.py:103 (Diff revision 9) > + except ObjectDoesNotExist: Please catch the specific Repository.DoesNotExist ::: pylib/mozreview/mozreview/resources/batch_review_request.py:108 (Diff revision 9) > + return self._no_access_error(user) Now that we're on 2.5.2 I'm pretty sure this method is gone and we should be using `self.get_no_access_erorr()` which takes the request object rather than the user object. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:123 (Diff revision 9) > + squashed_rr = ReviewRequest.objects.get(commit_id=identifier) You also need to specify the repository here or we're not guaranteed unique (we've been bitten by this bug before and had to fix it). ::: pylib/mozreview/mozreview/resources/batch_review_request.py:126 (Diff revision 9) > + return self._no_access_error(user) Same, update this ::: pylib/mozreview/mozreview/resources/batch_review_request.py:127 (Diff revision 9) > + except ObjectDoesNotExist: Please use `ReviewRequest.DoesNotExist` ::: pylib/mozreview/mozreview/resources/batch_review_request.py:148 (Diff revision 9) > + diffset = DiffSet.objects.create_from_data( > + repository=repo, > + diff_file_name='diff', > + diff_file_contents=commits['squashed']['diff'], > + parent_diff_file_name=None, > + parent_diff_file_contents=None, > + diffset_history=None, > + basedir='', > + request=request, > + base_commit_id=commits['squashed'].get('base_commit_id'), > + save=True > + ) In the future I think a big performance gain could be realized by moving all of the diffset creation code outside of the transaction. Diff parsing and processing can take quite a while for Review Board and once we are creating all of the commit requests in this transaction we could end up locking things for quite a while (with large diffs). Since diffsets can just hang around and not show up in the UI without being attached to a history it shouldn't cause any problems - and in the case of an error we could just manually clean them up. Maybe worth adding a TODO about this for the future? ::: pylib/mozreview/mozreview/resources/batch_review_request.py:161 (Diff revision 9) > + update_diffset_history(squashed_rr, diffset) Rather than do this you should be able to just pass the `diffset_history=squashed_rr.diffset_history` to `create_from_data()` above. You won't need to call save afterwards either. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:164 (Diff revision 9) > + except Exception: > + logger.exception('error processing parent diff') > + return INVALID_FORM_DATA, { > + 'fields': {'commits': 'error processing parent diff'}} I believe this breaks our transaction handling - Django transactions rely on an exception to roll things back. It appears to me you're going to end up with diffsets and review requests hanging around by doing this. What we'll want to do is throw exceptions up out of the transacation and catch them outside of it, then return our error response. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:165 (Diff revision 9) > + logger.exception('error processing parent diff') > + return INVALID_FORM_DATA, { > + 'fields': {'commits': 'error processing parent diff'}} Please refer to the "parent" diff as the "squashed" diff in code and logs. Review Board has a specific thing referred to as a parent diff and I want to avoid them ever getting confused. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:169 (Diff revision 9) > + discarded_diffset = None > + > + try: > + squashed_draft = squashed_rr.draft.get() > + if squashed_draft.diffset and squashed_draft.diffset != diffset: > + discarded_diffset = squashed_draft.diffset > + except ReviewRequestDraft.DoesNotExist: > + squashed_draft = ReviewRequestDraft.create(squashed_rr) > + > + squashed_draft.diffset = diffset > + squashed_draft.save() > + > + if discarded_diffset: > + discarded_diffset.delete() Please refactor this into a standalone function, it's definitely something reusable and as complexity grows it'll keep everything cleaner ::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:137 (Diff revision 9) > + batch_request_resource = api_root.get_extension( > + extension_name='mozreview.extension.MozReviewExtension')\ > + .get_batch_review_requests() I *think* this now can throw a bunch of `APIError`s that we don't catch anywhere and will spit a traceback to the pusher?
Attachment #8694470 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/26757/#review27041 > Rather than do this you should be able to just pass the `diffset_history=squashed_rr.diffset_history` to `create_from_data()` above. You won't need to call save afterwards either. Ah, okay, I realize what's going on here (You need to delete the current draft diffset before calculating the diff revision). In that case we should just pass `save=False` to the `create_from_data`, then delete the drafts diffset, then call save to perform the revision calculation for us.
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor https://reviewboard.mozilla.org/r/26783/#review27061 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:214 (Diff revision 8) > + # API object. No API objects here ::: pylib/mozreview/mozreview/resources/batch_review_request.py:301 (Diff revision 8) > + """Obtain a dictionary containing review metadata. When we're in RB extension land I'd **really** like to keep the distinction between "review" and "review request" crystal clear. I'd be very happy if you could fix any problems the pushhooks code has while porting it over. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:314 (Diff revision 8) > + rd['public'] = False > + except ReviewRequestDraft.DoesNotExist: > + rd['public'] = True I'm pretty sure this isn't always correct - a review requests public state is not `True` in all cases where there is no draft. I'm pretty sure you can have a review request where the draft doesn't exist but `reviewrequest.public == False`. I believe what you'll want when a draft doesn't exist is to set `rd['public'] = rr.public`. In the case of the draft existing, it *is* always `False` like you have: > Whether or not the draft is public. This will always be false up until the time it is first made public. At that point, the draft is deleted. This actually comes from the Web API though, since the draft model has no `public` field. So we're fine keeping the explicit False in the draft case.
Attachment #8694493 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/26757/#review27089 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:123 (Diff revision 9) > + squashed_rr = ReviewRequest.objects.get(commit_id=identifier) > + if not squashed_rr.is_mutable_by(user): > + logger.warn('%s not mutable by %s' % (squashed_rr.id, user)) Since we have direct access and are manipulating the review request directly instead of through the API we won't be prevented from updating requests which are closed / discarded etc. I think we need to have checks for these types of cases (I'm not sure if we test them all, and I know we have at least one case where the request is submitted that right now we just send back a traceback about the commit id being used.)
Attachment #8699070 - Flags: review?(smacleod)
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor https://reviewboard.mozilla.org/r/28177/#review27091 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:254 (Diff revision 9) > - user, repo, identifier, local_site, False) > + user, repo, identifier, local_site, True) This change shouldn't actually do anything except cause Review Board to run extra code, throw a `NotImplemenetedError` exception and then fallback and just set the `commit_id` field. If you're getting different behaviour with `True` vs `False` here I'm quite confused. We should stick with `False`. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:384 (Diff revision 9) > - #update_review_request(rr, commit) > + squashed_reviewers |= set(u for u in draft.target_people.all()) Personally I prefer calling the set method rather than the operator, plus the methods can take any iterable so we could just do `squashed_reviewers.update(u for u in draft.target_people.all())` ::: pylib/mozreview/mozreview/resources/batch_review_request.py:417 (Diff revision 9) > + assumed_old_rid = unclaimed_rids[0] > + unclaimed_rids.pop(0) IIRC `pop()` returns what it removes, we should just combine these two? Might as well do it now. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:437 (Diff revision 9) > + rr = ReviewRequest.objects.create(user, repo, None, local_site, True) It does nothing if you don't pass a `commit_id` (3rd argument) but pass `create_from_commit_id=True` (5th argument). We should stop passing `create_from_commit_id` here, it defaults to `False`. (Also, can we start specifying the argument names, I have to keep looking up the order / names). ::: pylib/mozreview/mozreview/resources/batch_review_request.py:447 (Diff revision 9) > + assert isinstance(rr.id, int) Why is this here? What else could it be? Can we get `None` here or something?? ::: pylib/mozreview/mozreview/resources/batch_review_request.py:513 (Diff revision 9) > + # TODO update reviewers on squashed draft. You do this above? and the post save hook we have on the draft updates our special field. Am I missing something or is this just a leftover comment? ::: pylib/mozreview/mozreview/resources/batch_review_request.py:548 (Diff revision 9) > + # TODO use review_helpers.gen_latest_reviews? Yes, do this, I consider it a blocker - I'd really like to keep these things consistent throughout. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:602 (Diff revision 9) > +def create_review_request_draft(rr): I really don't see what `ReviewRequestDraft.create()` isn't doing for us if we call it instead? The only variable I see things important hidden behind is `draft_is_new`, which if we're sure we're creating a draft and not updating one it will always be `True`. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:624 (Diff revision 9) > + draft.extra_data.update(rr.extra_data) This could cause problems with dictionaries within the dictionary. You should definitely deep copy. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:659 (Diff revision 9) > + logger.info('recognized reviewer: %s' % reviewer) unrecognized ::: pylib/mozreview/mozreview/resources/batch_review_request.py:685 (Diff revision 9) > + diffset_history=None, > + basedir='', > + request=request, > + base_commit_id=commit.get('base_commit_id'), > + save=True, We want `diffset_history` set properly here, then set `save=False`, then immediately after remove the draft diffset if it exists (which I think should be its own function), then finally save the diffset.
Attachment #8699612 - Flags: review?(smacleod) → review+
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod https://reviewboard.mozilla.org/r/28311/#review27117
https://reviewboard.mozilla.org/r/26757/#review27041 > I *think* this now can throw a bunch of `APIError`s that we don't catch anywhere and will spit a traceback to the pusher? It was like this before. That's why there are bugs with people seeing stacks from the server when doing `hg push`. This gets fixed as part of the refactor to switch the wire protocol to use JSON. I'm dropping this issue because it is existing.
Comment on attachment 8699769 [details] MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod https://reviewboard.mozilla.org/r/28437/#review27121
Attachment #8699769 - Flags: review?(smacleod) → review+
Comment on attachment 8699770 [details] MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod https://reviewboard.mozilla.org/r/28439/#review27129 The reason for the old RBTools version was unicode problems which should all have been fixed now.
Attachment #8699770 - Flags: review?(smacleod) → review+
Comment on attachment 8699771 [details] MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod https://reviewboard.mozilla.org/r/28441/#review27133
Attachment #8699771 - Flags: review?(smacleod) → review+
Comment on attachment 8699772 [details] MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod https://reviewboard.mozilla.org/r/28443/#review27135
Attachment #8699772 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/26757/#review27041 > It was like this before. That's why there are bugs with people seeing stacks from the server when doing `hg push`. This gets fixed as part of the refactor to switch the wire protocol to use JSON. I'm dropping this issue because it is existing. I moreso meant exceptions due to conditions which we used to handle, but now they'll be coming down from Review Board as an HTTP code that will throw on us. I don't have anything concrete though, and if the tests pass I guess we can just patch them up case by case.
Attachment #8699774 - Flags: review?(smacleod) → review+
Comment on attachment 8699774 [details] MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod https://reviewboard.mozilla.org/r/28447/#review27147
https://reviewboard.mozilla.org/r/28177/#review27091 > Why is this here? What else could it be? Can we get `None` here or something?? I think this is left over from client-side foo and RBTools. We're obviously not hitting it or we'd have a bug report on file. I'll remove it.
Comment on attachment 8699773 [details] MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/3-4/
Attachment #8699773 - Attachment description: MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r?smacleod → MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod
Comment on attachment 8699069 [details] MozReview Request: mozreview: print more diff info; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/5-6/
Attachment #8699069 - Attachment description: MozReview Request: mozreview: print more diff info; r?smacleod → MozReview Request: mozreview: print more diff info; r=smacleod
Comment on attachment 8699565 [details] MozReview Request: mozreview: log user events; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/4-5/
Attachment #8699565 - Attachment description: MozReview Request: mozreview: log user events; r?smacleod → MozReview Request: mozreview: log user events; r=smacleod
We swallow output because it currently spews an exception stack, which is a bug already on file. Review commit: https://reviewboard.mozilla.org/r/30437/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30437/
Attachment #8706702 - Flags: review?(smacleod)
Attachment #8694470 - Flags: review?(smacleod)
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/9-10/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/8-9/
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/9-10/
Attachment #8699070 - Flags: review?(smacleod)
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/6-7/
Attachment #8699612 - Attachment description: MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r?smacleod → MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod
Comment on attachment 8699769 [details] MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/4-5/
Attachment #8699769 - Attachment description: MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r?smacleod → MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod
Comment on attachment 8699770 [details] MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/4-5/
Attachment #8699770 - Attachment description: MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r?smacleod → MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod
Comment on attachment 8699771 [details] MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/4-5/
Attachment #8699771 - Attachment description: MozReview Request: reviewboard: use built-in mechanism to disable caching; r?smacleod → MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod
Comment on attachment 8699772 [details] MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/4-5/
Attachment #8699772 - Attachment description: MozReview Request: reviewboard: use built-in mechanism to not save cookies; r?smacleod → MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod
Comment on attachment 8699774 [details] MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/4-5/
Attachment #8699774 - Attachment description: MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r?smacleod → MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod
Attachment #8706702 - Flags: review?(smacleod) → review+
Comment on attachment 8706702 [details] MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod https://reviewboard.mozilla.org/r/30437/#review27323
Comment on attachment 8699773 [details] MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/4-5/
Comment on attachment 8699069 [details] MozReview Request: mozreview: print more diff info; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/6-7/
Comment on attachment 8699565 [details] MozReview Request: mozreview: log user events; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/5-6/
Comment on attachment 8706702 [details] MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30437/diff/1-2/
Attachment #8706702 - Attachment description: MozReview Request: reviewboard: add test for pushing to submitted review request; r?smacleod → MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod
We didn't have explicit test coverage of this before. Review commit: https://reviewboard.mozilla.org/r/30607/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30607/
Attachment #8707190 - Flags: review?(smacleod)
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/10-11/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/9-10/
Attachment #8694493 - Attachment description: MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r?smacleod, dminor → MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/10-11/
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/7-8/
Comment on attachment 8699769 [details] MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/5-6/
Comment on attachment 8699770 [details] MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/5-6/
Comment on attachment 8699771 [details] MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/5-6/
Comment on attachment 8699772 [details] MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/5-6/
Comment on attachment 8699774 [details] MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/5-6/
https://reviewboard.mozilla.org/r/26755/#review27405 No matter how much I try, I can't figure out how to handle the diffset manipulation in the way smacleod suggests. I've tried a ton of different combinations of API calls and nothing seems to work. I'm inclined to push what I have working and leave refactoring the diffset code to a follow-up bug.
Comment on attachment 8699773 [details] MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/5-6/
Comment on attachment 8699069 [details] MozReview Request: mozreview: print more diff info; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/7-8/
Comment on attachment 8699565 [details] MozReview Request: mozreview: log user events; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/6-7/
Comment on attachment 8706702 [details] MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30437/diff/2-3/
Comment on attachment 8707190 [details] MozReview Request: reviewboard: add test for pushing to a discarded series; r?smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30607/diff/1-2/
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/11-12/
Comment on attachment 8694493 [details] MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/10-11/
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/11-12/
Comment on attachment 8699612 [details] MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/8-9/
Comment on attachment 8699769 [details] MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/6-7/
Comment on attachment 8699770 [details] MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/6-7/
Comment on attachment 8699771 [details] MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/6-7/
Comment on attachment 8699772 [details] MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/6-7/
Comment on attachment 8699774 [details] MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/6-7/
This is the proper way to temporarily deactivate demandimport, as it will restore the previous state instead of assuming it was enabled to begin with. Review commit: https://reviewboard.mozilla.org/r/30625/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30625/
Attachment #8707257 - Flags: review?(smacleod)
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor https://reviewboard.mozilla.org/r/28177/#review27507 lgtm ::: pylib/mozreview/mozreview/resources/batch_review_request.py:165 (Diff revision 12) > + * TODO Bug 1047465 Bug 1047465 is resolved fixed. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:581 (Diff revision 12) > + # consistent. For now, we duplicate that logic here. I think you've fixed this TODO :)
Attachment #8699070 - Flags: review?(dminor) → review+
Comment on attachment 8707190 [details] MozReview Request: reviewboard: add test for pushing to a discarded series; r?smacleod https://reviewboard.mozilla.org/r/30607/#review27533
Attachment #8707190 - Flags: review?(smacleod) → review+
Blocks: 1195661
Comment on attachment 8694470 [details] MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor https://reviewboard.mozilla.org/r/26757/#review27535 Ya, it's not a huge deal the way you have the diffset processing. I agree, we can just poke at it after we've landed this stuff. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:38 (Diff revision 12) > +class SquashedDiffProcessingException(Exception): > + pass In future iterations of your series you don't special case any other diffset processing errors? We should probably have a genric one for diffsets and actually check for all of them. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:128 (Diff revision 12) > + except SquashedDiffProcessingException: > + return INVALID_FORM_DATA, { > + 'fields': {'commits': 'error processing squashed diff'}} There might be other exceptions we should special case to return proper errors instead of just a 500, we can iterate on that though. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:167 (Diff revision 12) > + # Calling create_from_data() instead of create_from_upload() skips > + # diff size validation. We allow unlimited diff sizes, so no biggie. > + try: Please move this comment inside the try block to the actaul `create_from_data` call. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:189 (Diff revision 12) > + logger.exception('error processing squashed diff') I'd prefer if we logged a little more information about what the exception actually was.
Attachment #8694470 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/26757/#review27041 > Ah, okay, I realize what's going on here (You need to delete the current draft diffset before calculating the diff revision). In that case we should just pass `save=False` to the `create_from_data`, then delete the drafts diffset, then call save to perform the revision calculation for us. Dropping this, we can iterate.
Comment on attachment 8699070 [details] MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor https://reviewboard.mozilla.org/r/28177/#review27541 ::: pylib/mozreview/mozreview/resources/batch_review_request.py:42 (Diff revision 12) > +from ..review_helpers import ( > + gen_latest_reviews, > +) I'd much rather we just call out the entire import path, I don't think we're doing this anywhere else. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:670 (Diff revision 12) > + diffset = DiffSet.objects.create_from_data( > + repository=rr.repository, > + diff_file_name='diff', > + diff_file_contents=commit['diff'], > + parent_diff_file_name='diff', > + parent_diff_file_contents=commit.get('parent_diff'), > + diffset_history=None, > + basedir='', > + request=request, > + base_commit_id=commit.get('base_commit_id'), > + save=True, > + ) Catch / Throw a diffset processing exception so we can 400 instead of 500. ::: pylib/mozreview/mozreview/resources/batch_review_request.py:685 (Diff revision 12) > + discarded_diffset = None > + if draft.diffset and draft.diffset != diffset: > + discarded_diffset = draft.diffset > + > + draft.diffset = diffset > + draft.save() > + > + if discarded_diffset: > + discarded_diffset.delete() This logic should really be merged into `update_review_request_draft_diffset` in a way that you can just re-use / call it here too.
Attachment #8699070 - Flags: review?(smacleod) → review+
Comment on attachment 8707257 [details] MozReview Request: reviewboard: use demandimport.deactivated; r?smacleod https://reviewboard.mozilla.org/r/30625/#review27547
Attachment #8707257 - Flags: review?(smacleod) → review+
https://hg.mozilla.org/hgcustom/version-control-tools/rev/44901a8bbcd8e06252578b171b2c6c5271b6023a mozreview: add web API to submit an entire series of commits (bug 1229468); r=smacleod, dminor https://hg.mozilla.org/hgcustom/version-control-tools/rev/64bc74f4b91a7730c7d03c4fcbe9f4a723813a75 mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor https://hg.mozilla.org/hgcustom/version-control-tools/rev/252a96904fba6c1d1bc11caf6a40bf6d09dfe3d7 mozreview: create child review requests from batch API (bug 1229468); r=smacleod, dminor https://hg.mozilla.org/hgcustom/version-control-tools/rev/1cc42ef4b24f5b9e25f737ceba51ddc0ef588ea1 mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod
Deployed to prod.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1239452
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: