Closed
Bug 1191817
Opened 9 years ago
Closed 9 years ago
Changes leftover from bug 1109218
Categories
(MozReview Graveyard :: General, defect, P2)
MozReview Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mdoglio, Assigned: mdoglio)
References
Details
Attachments
(4 files)
In bug 1109218 there are some comments :smacleod put as a post-merge review. Let's fix them in this bug: +++ b/pylib/mozreview/mozreview/autoland/resources.py > +import json > +import logging Alphebetical order. +++ b/pylib/mozreview/mozreview/autoland/resources.py > +from mozreview.autoland.models import (AutolandEventLogEntry, > + AutolandRequest) > +from mozreview.autoland.errors import (AUTOLAND_ERROR, > + AUTOLAND_TIMEOUT, > + BAD_AUTOLAND_CREDENTIALS, > + BAD_AUTOLAND_URL, > + BAD_UPDATE_CREDENTIALS) Alphabetical order. +++ b/pylib/mozreview/mozreview/autoland/resources.py > + """Resource for reviewers or reviewees to kick off Try Builds > + for a particular review request.""" "The summary line may be used by automatic indexing tools; it is important that it fits on one line and is separated from the rest of the docstring by a blank line" https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings +++ b/pylib/mozreview/mozreview/autoland/resources.py > + def has_access_permissions(self, request, *args, **kwargs): > + return False I'm not sure why we are allowing 'GET' when we never give anyone permission? If we really don't want to define a get, or allow anyone to use it we should probably remove it from the `allowed_methods` above. +++ b/pylib/mozreview/mozreview/autoland/resources.py > + optional={ > + 'autoland_request_id_for_testing': { > + 'type': int, > + 'description': 'For testing only. If the MozReview extension ' > + 'is in testing mode, this skips the request to ' > + 'Try Autoland and just uses this request id.' > + } > + } Now that we have proper support for testing autoland as part of our infrastructure I wonder if it's worth tearing this out (along with all the other testing flags)? +++ b/pylib/mozreview/mozreview/autoland/resources.py > + try: > + autoland_request_id = int(response.json().get('request_id', 0)) > + finally: > + if autoland_request_id is None: > + return AUTOLAND_ERROR, { > + 'status_code': response.status_code, > + 'request_id': autoland_request_id, > + } Should we even bother including request_id in the returned error, it will always be None? Or should we setting a literal None to that key instead of the var? +++ b/pylib/mozreview/mozreview/autoland/resources.py > + """Resource for notifications of Autoland requests status update""" Missing period. +++ b/pylib/mozreview/mozreview/autoland/resources.py > + def has_access_permissions(self, request, *args, **kwargs): > + return False I don't think this is needed, 'GET' isn't in allowed_methods. +++ b/pylib/mozreview/mozreview/autoland/resources.py > + # TODO - remove this before landing. Just for local testing on mconley's machine > + #return "http://192.168.59.3:60353/api/extensions/mozreview.extension.MozReviewExtension/autoland-request-updates/" Self explanatory. --- a/pylib/rbmozui/rbmozui/fields.py +++ b/pylib/rbmozui/rbmozui/fields.py > +from mozreview.utils import is_parent, is_pushed > + > +from mozreview.autoland.models import AutolandRequest, AutolandEventLogEntry No blank space, alphabetical. +++ b/pylib/rbmozui/rbmozui/static/js/try.js > + .done(function(){ > + // TODO > + }) What do we need to do here?
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mdoglio
Updated•9 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•9 years ago
|
||
Mozreview: fix import order (bug 1191817); r?smacleod
Attachment #8651783 -
Flags: review?(smacleod)
Assignee | ||
Comment 2•9 years ago
|
||
Mozreview: fix autoland resources docstrings (bug 1191817); r?smacleod
Attachment #8651784 -
Flags: review?(smacleod)
Assignee | ||
Comment 3•9 years ago
|
||
Mozreview: remove GET access to TryAutolandTriggerResource (bug 1191817); r?smacleod
Attachment #8651785 -
Flags: review?(smacleod)
Assignee | ||
Comment 4•9 years ago
|
||
Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod
Attachment #8651786 -
Flags: review?(smacleod)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 5•9 years ago
|
||
Comment on attachment 8651783 [details] MozReview Request: Mozreview: fix import order (bug 1191817); r?smacleod https://reviewboard.mozilla.org/r/16971/#review15219 Ship It!
Attachment #8651783 -
Flags: review?(smacleod) → review+
Updated•9 years ago
|
Attachment #8651784 -
Flags: review?(smacleod) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8651784 [details] MozReview Request: Mozreview: fix autoland resources docstrings (bug 1191817); r?smacleod https://reviewboard.mozilla.org/r/16973/#review15221 ::: pylib/mozreview/mozreview/autoland/resources.py:42 (Diff revision 1) > - """Resource for reviewers or reviewees to kick off Try Builds > - for a particular review request.""" > + """Resource to kick off Try Builds for a particular review request.""" > + > + """Only reviewers or reviewees can start a Try Build.""" In a multiline docstring you'll want the whole thing contained between one set of `""" ... """`. The first line contains `"""` as well as text, but the last line should be the `"""` alone. So something like: ``` def function(): """I'm the summary line I'm another paragraph. Lots of text here and here and here. Ooo, maybe another paragraph. """ ``` ::: pylib/mozreview/mozreview/autoland/resources.py:44 (Diff revision 1) > + """Only reviewers or reviewees can start a Try Build.""" Is this true, I thought it was authors only atm?
Comment 7•9 years ago
|
||
Comment on attachment 8651785 [details] MozReview Request: Mozreview: remove GET access to TryAutolandTriggerResource (bug 1191817); r?smacleod https://reviewboard.mozilla.org/r/16975/#review15225 ::: pylib/mozreview/mozreview/autoland/resources.py:47 (Diff revision 1) > - allowed_methods = ('GET', 'POST',) > + allowed_methods = ('POST',) I just want to make sure you confirmed our front-end still works with this - if so please drop this issue :)
Attachment #8651785 -
Flags: review?(smacleod) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8651786 [details] MozReview Request: Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod https://reviewboard.mozilla.org/r/16977/#review15227 Ship It!
Attachment #8651786 -
Flags: review?(smacleod) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8651786 -
Flags: review+ → review?(smacleod)
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8651786 [details] MozReview Request: Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod
Assignee | ||
Comment 10•9 years ago
|
||
https://reviewboard.mozilla.org/r/16975/#review15225 > I just want to make sure you confirmed our front-end still works with this - if so please drop this issue :) Verified with a manual test
Assignee | ||
Updated•9 years ago
|
Attachment #8651786 -
Flags: review?(smacleod)
Assignee | ||
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Developer Services → MozReview
You need to log in
before you can comment on or make changes to this bug.
Description
•