Review Board Web API to submit a series of review requests

RESOLVED FIXED

Status

MozReview
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 1 bug)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(15 attachments)

40 bytes, text/x-review-board-request
dminor
: review+
smacleod
: review+
Details | Review
40 bytes, text/x-review-board-request
smacleod
: review+
dminor
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
dminor
: review+
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
58 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

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)
(Assignee)

Comment 2

2 years ago
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/
(Assignee)

Comment 3

2 years ago
Created attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

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)
(Assignee)

Comment 4

2 years ago
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 :/
(Assignee)

Comment 5

2 years ago
Created attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

Review commit: https://reviewboard.mozilla.org/r/27509/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/27509/
Attachment #8699069 - Flags: review?(smacleod)
(Assignee)

Comment 6

2 years ago
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/
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Comment 8

2 years ago
Created attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review commit: https://reviewboard.mozilla.org/r/28177/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28177/
(Assignee)

Comment 9

a year ago
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/
(Assignee)

Comment 10

a year ago
Created attachment 8699565 [details]
MozReview Request: mozreview: log user events; r=smacleod

Review commit: https://reviewboard.mozilla.org/r/28277/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28277/
Attachment #8699565 - Flags: review?(smacleod)
(Assignee)

Comment 11

a year ago
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/
(Assignee)

Comment 12

a year ago
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/
(Assignee)

Comment 13

a year ago
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/
(Assignee)

Comment 14

a year ago
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)
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 15

a year ago
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/
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 16

a year ago
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/
(Assignee)

Comment 17

a year ago
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
(Assignee)

Comment 18

a year ago
Created attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Review commit: https://reviewboard.mozilla.org/r/28311/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28311/
Attachment #8699612 - Flags: review?(smacleod)
(Assignee)

Comment 19

a year ago
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/
(Assignee)

Comment 20

a year ago
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/
(Assignee)

Comment 21

a year ago
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/
(Assignee)

Comment 22

a year ago
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/
(Assignee)

Comment 23

a year ago
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/
(Assignee)

Comment 24

a year ago
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/
(Assignee)

Comment 25

a year ago
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/
(Assignee)

Comment 26

a year ago
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/
(Assignee)

Comment 27

a year ago
Created attachment 8699769 [details]
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

Review commit: https://reviewboard.mozilla.org/r/28437/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28437/
Attachment #8699769 - Flags: review?(smacleod)
(Assignee)

Comment 28

a year ago
Created attachment 8699770 [details]
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

Review commit: https://reviewboard.mozilla.org/r/28439/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28439/
Attachment #8699770 - Flags: review?(smacleod)
(Assignee)

Comment 29

a year ago
Created attachment 8699771 [details]
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

Review commit: https://reviewboard.mozilla.org/r/28441/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28441/
Attachment #8699771 - Flags: review?(smacleod)
(Assignee)

Comment 30

a year ago
Created attachment 8699772 [details]
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

Review commit: https://reviewboard.mozilla.org/r/28443/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28443/
Attachment #8699772 - Flags: review?(smacleod)
(Assignee)

Comment 31

a year ago
Created attachment 8699773 [details]
MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod

Review commit: https://reviewboard.mozilla.org/r/28445/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28445/
Attachment #8699773 - Flags: review?(smacleod)
(Assignee)

Comment 32

a year ago
Created attachment 8699774 [details]
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

Review commit: https://reviewboard.mozilla.org/r/28447/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28447/
Attachment #8699774 - 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

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+

Updated

a year ago
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

Updated

a year ago
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
(Assignee)

Comment 36

a year ago
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
(Assignee)

Comment 37

a year ago
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/
(Assignee)

Comment 38

a year ago
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/
(Assignee)

Comment 39

a year ago
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/
(Assignee)

Comment 40

a year ago
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/
(Assignee)

Updated

a year ago
Attachment #8699070 - Flags: review?(dminor)
(Assignee)

Comment 41

a year ago
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/
(Assignee)

Comment 42

a year ago
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/
(Assignee)

Comment 43

a year ago
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/
(Assignee)

Comment 44

a year ago
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/
(Assignee)

Comment 45

a year ago
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/
(Assignee)

Comment 46

a year ago
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/
(Assignee)

Comment 47

a year ago
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/
(Assignee)

Comment 48

a year ago
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/
(Assignee)

Comment 49

a year ago
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/
(Assignee)

Comment 50

a year ago
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/
(Assignee)

Comment 51

a year ago
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/
(Assignee)

Comment 52

a year ago
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/
(Assignee)

Comment 53

a year ago
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/
(Assignee)

Comment 54

a year ago
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/
(Assignee)

Comment 55

a year ago
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/
(Assignee)

Comment 56

a year ago
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/
(Assignee)

Comment 57

a year ago
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.

Updated

a year ago
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?
(Assignee)

Comment 59

a year ago
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/
(Assignee)

Comment 60

a year ago
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/
(Assignee)

Comment 61

a year ago
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/
(Assignee)

Comment 62

a year ago
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/
(Assignee)

Comment 63

a year ago
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/
(Assignee)

Comment 64

a year ago
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)
(Assignee)

Comment 65

a year ago
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/
(Assignee)

Comment 66

a year ago
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/
(Assignee)

Comment 67

a year ago
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/
(Assignee)

Comment 68

a year ago
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/
(Assignee)

Comment 69

a year ago
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/
(Assignee)

Comment 70

a year ago
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
(Assignee)

Comment 80

a year ago
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
(Assignee)

Comment 87

a year ago
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.
(Assignee)

Comment 88

a year ago
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
(Assignee)

Comment 89

a year ago
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
(Assignee)

Comment 90

a year ago
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
(Assignee)

Comment 91

a year ago
Created attachment 8706702 [details]
MozReview Request: reviewboard: add test for pushing to submitted review request; 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)
(Assignee)

Updated

a year ago
Attachment #8694470 - Flags: review?(smacleod)
(Assignee)

Comment 92

a year ago
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/
(Assignee)

Comment 93

a year ago
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/
(Assignee)

Comment 94

a year ago
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)
(Assignee)

Comment 95

a year ago
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
(Assignee)

Comment 96

a year ago
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
(Assignee)

Comment 97

a year ago
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
(Assignee)

Comment 98

a year ago
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
(Assignee)

Comment 99

a year ago
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
(Assignee)

Comment 100

a year ago
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
(Assignee)

Comment 102

a year ago
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/
(Assignee)

Comment 103

a year ago
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/
(Assignee)

Comment 104

a year ago
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/
(Assignee)

Comment 105

a year ago
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
(Assignee)

Comment 106

a year ago
Created attachment 8707190 [details]
MozReview Request: reviewboard: add test for pushing to a discarded series; 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)
(Assignee)

Comment 107

a year ago
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/
(Assignee)

Comment 108

a year ago
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
(Assignee)

Comment 109

a year ago
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/
(Assignee)

Comment 110

a year ago
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/
(Assignee)

Comment 111

a year ago
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/
(Assignee)

Comment 112

a year ago
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/
(Assignee)

Comment 113

a year ago
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/
(Assignee)

Comment 114

a year ago
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/
(Assignee)

Comment 115

a year ago
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/
(Assignee)

Comment 116

a year ago
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.
(Assignee)

Comment 117

a year ago
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/
(Assignee)

Comment 118

a year ago
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/
(Assignee)

Comment 119

a year ago
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/
(Assignee)

Comment 120

a year ago
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/
(Assignee)

Comment 121

a year ago
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/
(Assignee)

Comment 122

a year ago
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/
(Assignee)

Comment 123

a year ago
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/
(Assignee)

Comment 124

a year ago
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/
(Assignee)

Comment 125

a year ago
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/
(Assignee)

Comment 126

a year ago
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/
(Assignee)

Comment 127

a year ago
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/
(Assignee)

Comment 128

a year ago
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/
(Assignee)

Comment 129

a year ago
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/
(Assignee)

Comment 130

a year ago
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/
(Assignee)

Comment 131

a year ago
Created attachment 8707257 [details]
MozReview Request: reviewboard: use demandimport.deactivated; r?smacleod

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)
https://reviewboard.mozilla.org/r/26757/#review27505

lgtm
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+

Updated

a year ago
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+
(Assignee)

Comment 139

a year ago
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
(Assignee)

Comment 140

a year ago
Deployed to prod.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Blocks: 1239452
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.