1109218 - UI for Autoland/hg transplant-to-try in MozReview

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Mark Côté [:mcote]]

A big step in integrating MozReview further into the build process is to provide the ability from Review Board to send a commit set to Try.

For the UI, we'll need something like the TryChooser.

For the back end, we will need something to transplant the commits to try and something to post the results back.  This may involve Autoland or just the transplant tool directly.  dminor is the lead on Autoland.

Comment 1

[Mark Côté [:mcote]]

Correcting this bug.  We just need a *link* to TryChooser, so we don't duplicate its functionality.  MozReview's UI should just take a try commit string.

Comment 2

[Mark Côté [:mcote]]

Also this bug is really very separate two parts: the UI, and the back end (Autoland service).  Changing this to be the former.

Comment 3

[Mark Côté [:mcote]]

Back end/service filed as bug 1121616.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: bz://1109218/mconley

Comment 5

[Mike Conley (:mconley) (:⚙️)]

/r/2677 - rbmozui: Refactor fields.py to bring out some common functions.
/r/2679 - rbmozui: Add initial Try field.
/r/2681 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2683 - rbmozui: Adding try.js

Pull down these commits:

hg pull review -r 46c54cb2df6bea8030fae76aa415147f7475525e

Comment 6

[Mike Conley (:mconley) (:⚙️)]

/r/2677 - rbmozui: Refactor fields.py to bring out some common functions.
/r/2679 - rbmozui: Add initial Try field.
/r/2681 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2683 - rbmozui: Adding try.js
/r/2691 - Start adding a Try Build dialog

Pull down these commits:

hg pull review -r e0f46936f451830efeac0013a529cf2cfeeb7308

Comment 7

[Mike Conley (:mconley) (:⚙️)]

/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.

Pull down this commit:

hg pull review -r 09be80f5d398b5fa92576eeb306f00b96bde0118

Comment 8

[Mike Conley (:mconley) (:⚙️)]

/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2707 - Start adding a Try Build dialog
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.

Pull down these commits:

hg pull review -r 09be80f5d398b5fa92576eeb306f00b96bde0118

Comment 9

[Mike Conley (:mconley) (:⚙️)]

/r/2717 - rbmozui: Refactor fields.py to bring out some common functions.
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2707 - Start adding a Try Build dialog
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.
/r/2719 - Aesthetic cleanups, removal of dead code.
/r/2721 - Starting support for communicating with autoland.

Pull down these commits:

hg pull review -r b3d46dc667de7906355d85412b0e26f411721bf5

Comment 10

[Mike Conley (:mconley) (:⚙️)]

/r/2717 - rbmozui: Refactor fields.py to bring out some common functions.
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2707 - Start adding a Try Build dialog
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.
/r/2719 - Aesthetic cleanups, removal of dead code.
/r/2721 - Starting support for communicating with autoland.
/r/2725 - Point to the right autoland endpoint, and some other niceties.

Pull down these commits:

hg pull review -r f48a1a1d18573f75843c09371df1f3b7d5777aa8

Comment 11

[Mike Conley (:mconley) (:⚙️)]

/r/2717 - rbmozui: Refactor fields.py to bring out some common functions.
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2707 - Start adding a Try Build dialog
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.
/r/2719 - Aesthetic cleanups, removal of dead code.
/r/2721 - Starting support for communicating with autoland.
/r/2725 - Point to the right autoland endpoint, and some other niceties.
/r/2745 - Add an untested webapi endpoing for the Try Autoland UI to hit.

Pull down these commits:

hg pull review -r 95a5a386f49cd2213f1842505791521ebc30a780

Comment 12

[Mike Conley (:mconley) (:⚙️)]

/r/2747 - mozreview: Add barebones extension and packaging (Bug 1091381)
/r/2749 - mozreview: Integrate mozreview into the testing and development machinery.
/r/2751 - mozreview: Introduce module for listening to signals.
/r/2753 - mozreview: Add form to configure Mozilla Pulse settings.
/r/2755 - mozreview: Only execute signal handlers if settings.enabled is True
/r/2757 - mozreview: Publish review_request_published to Pulse.
/r/2717 - rbmozui: Refactor fields.py to bring out some common functions.
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2707 - Start adding a Try Build dialog
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.
/r/2719 - Aesthetic cleanups, removal of dead code.
/r/2721 - Starting support for communicating with autoland.
/r/2725 - Point to the right autoland endpoint, and some other niceties.
/r/2745 - Add an untested webapi endpoing for the Try Autoland UI to hit.

Pull down these commits:

hg pull review -r 32194811e788b432ab87aecd6cc7ab05d07d97db

Comment 13

[Mike Conley (:mconley) (:⚙️)]

/r/2747 - mozreview: Add barebones extension and packaging (Bug 1091381)
/r/2749 - mozreview: Integrate mozreview into the testing and development machinery.
/r/2751 - mozreview: Introduce module for listening to signals.
/r/2753 - mozreview: Add form to configure Mozilla Pulse settings.
/r/2755 - mozreview: Only execute signal handlers if settings.enabled is True
/r/2757 - mozreview: Publish review_request_published to Pulse.
/r/2717 - rbmozui: Refactor fields.py to bring out some common functions.
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2707 - Start adding a Try Build dialog
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.
/r/2719 - Aesthetic cleanups, removal of dead code.
/r/2721 - Starting support for communicating with autoland.
/r/2725 - Point to the right autoland endpoint, and some other niceties.
/r/2745 - Add an untested webapi endpoing for the Try Autoland UI to hit.
/r/2759 - Move untested TryAutolandTriggerResource to the MozReview extension.

Pull down these commits:

hg pull review -r 3dbdfdc72675df3125e06ee6a23d58a52334a5a3

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2717 - rbmozui: Refactor fields.py to bring out some common functions.
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2707 - Start adding a Try Build dialog
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.
/r/2719 - Aesthetic cleanups, removal of dead code.
/r/2721 - Starting support for communicating with autoland.
/r/2725 - Point to the right autoland endpoint, and some other niceties.
/r/2745 - Add an untested webapi endpoing for the Try Autoland UI to hit.
/r/2759 - Move untested TryAutolandTriggerResource to the MozReview extension.
/r/2747 - Get the resource properly registered
/r/2749 - Fix up the resource to work with manual test case.
/r/2751 - Get try.js to talk to our new API endpoint.

Pull down these commits:

hg pull review -r 19b4a87379ed92bf585b9570aaf3b9ab279b0e22

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2717 - Bug 1109218 - Part 1: rbmozui: Refactor fields.py to bring out some common functions. r=?
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2707 - Start adding a Try Build dialog
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2677 - Small fiddly changes to start communicating with the root review request when the job has been scheduled.
/r/2719 - Aesthetic cleanups, removal of dead code.
/r/2721 - Starting support for communicating with autoland.
/r/2725 - Point to the right autoland endpoint, and some other niceties.
/r/2745 - Add an untested webapi endpoing for the Try Autoland UI to hit.
/r/2759 - Move untested TryAutolandTriggerResource to the MozReview extension.
/r/2747 - Get the resource properly registered
/r/2749 - Fix up the resource to work with manual test case.
/r/2751 - Get try.js to talk to our new API endpoint.
/r/2803 - Make the Try Autoland UI for RBMozUI toggle-able
/r/2805 - Add Try Autoland URL configuration to MozReview.

Pull down these commits:

hg pull review -r 9f548eba57a82b9b94082665e206df8ae50e831d

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2717 - Bug 1109218 - Part 1: rbmozui: Refactor fields.py to bring out some common functions. r=?
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2721 - Starting support for communicating with autoland.
/r/2751 - Get try.js to talk to our new API endpoint.
/r/2749 - Add an untested webapi endpoint for the Try Autoland UI to hit.
/r/2803 - Make the Try Autoland UI for RBMozUI toggle-able
/r/2805 - Add Try Autoland URL configuration to MozReview.

Pull down these commits:

hg pull review -r 03d7acc0e7e382ef55c1296a2e761128301e87fa

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2717 - Bug 1109218 - Part 1: rbmozui: Refactor fields.py to bring out some common functions. r=?
/r/2701 - rbmozui: Add initial Try field.
/r/2703 - rbmozui: Make review-scripts-js.html the main entry point for the review request page JS
/r/2705 - rbmozui: Adding try.js
/r/2709 - rbmozui: Make getting the root review request into a common place, since we use it in various places.
/r/2711 - mdoglio updates
/r/2721 - Starting support for communicating with autoland.
/r/2751 - Get try.js to talk to our new API endpoint.
/r/2749 - Add an untested webapi endpoint for the Try Autoland UI to hit.
/r/2803 - Make the Try Autoland UI for RBMozUI toggle-able
/r/2805 - Add Try Autoland URL configuration to MozReview.

Pull down these commits:

hg pull review -r 03d7acc0e7e382ef55c1296a2e761128301e87fa

Comment 18

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2717/#review1967

Ship It!

Comment 19

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2703/#review1971

Ship It!

Comment 20

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2701/#review1973

Ship It!

Comment 21

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2705/#review1975

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 5)
> +      title: "Trigger a try build",

Try should be capitalized.

Comment 22

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2709/#review1977

::: pylib/rbmozui/rbmozui/static/js/common.js
(Diff revision 5)
> +  var rootID = $("#rbmozui-commits-root").data("id");

Where is `#rbmozui-commits-root` getting put into the DOM?

Comment 23

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2709/#review1979

> Where is `#rbmozui-commits-root` getting put into the DOM?

The CommitList that's displayed at the top of each push2rb'd review request injects this value when it inserts its Field. See rbmozui/rbmozui/fields.py and rbmozui/rbmozui/templates/rbmozui/commits.html.

Comment 24

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2709/#review1981

Ship It!

Comment 25

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2805/#review1983

Ship It!

Comment 26

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2803/#review2003

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 2)
> +        if not ext or not ext.settings.get('autoland_try_ui_enabled', False):

By default won't `x.get('blah')` return `None` if `'blah' not in x` and `None` is "falsy", so are you just doing this for clarity?

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 2)
> -

Why this change?

::: pylib/rbmozui/rbmozui/forms.py
(Diff revision 2)
> +from django.utils.translation import ugettext as _

You import this and don't use it.

Comment 27

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2717/#review2007

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 4)
> +def isP2RB(review_request):
> +    return str(review_request.extra_data.get('p2rb', False)).lower() == "true"

We'll want to use mozreview.utils.is_pushed() for this eventually.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 4)
> +def getRoot(review_request):

Again we'll want to move this into mozreview.utils

Comment 28

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2749/#review2005

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +    name = 'try-autoland-trigger'

As a convention, names should contain underscrores. For mimetypes, etc., they get autoconverted to hyphens.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +    def post(self, request, review_request_id, try_syntax, *args, **kwargs):

This should be called `create` not `post`.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +    @webapi_login_required

Do we want to support `@webapi_check_local_site` for generality's sake, or is it not worth worrying about?

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        if (str(rr.extra_data.get('p2rb', False)).lower() != "true" or
> +            str(rr.extra_data.get('p2rb.is_squashed', False)).lower() != "true"):

You should use `django.utils.six` and use `six.text_type` for compatablity.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +import json

`from __future__ import unicode_literals`

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 4)
> +from django.db import models

`from __future__ import unicode_literals`

::: pylib/mozreview/mozreview/extension.py
(Diff revision 4)
>  from reviewboard.extensions.base import Extension

You should have

`from __future__ import unicode_literals`

because ALL of Review Board is going to be expecting unicode strings UNLESS you really know you shouldn't be doing it. If so, you may want to put a comment as to why OR use the `b''` prefix.

Comment 29

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2749/#review2011

As a style note, you're all over the place with `'` vs `"`

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +

You may want to define the following so that unauhtorized users can't see this resource and get a 403:

- `has_access_permissions`
- `has_modify_permisisons`
- `has_list_access_permissions`

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +import requests

`requests` is 3rd party so it should be lumped in with the `django`/`djblets` stuff

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +

No blank line. `django` and `djblets` are both 3rd-party.

Comment 30

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2751/#review2015

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 4)
>                var activityIndicator = $("#activity-indicator")
>                  .removeClass("error")
>                  .text(gettext("Scheduling jobs..."))
>                  .show();

Instead of manually setting the activity indicator, you may want to use `RB.setActivityIndicator` (https://github.com/reviewboard/reviewboard/blob/ea775c5e7b32ee427101e9e1f886b1c0398f9170/reviewboard/static/rb/js/utils/apiUtils.js). I don't know if it will fit your needs completely, so feel free to drop this issue.

Comment 31

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2701/#review2009

::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 4)
> +        # The above hack forced Commits at the top, but the rest of these fields
> +        # are fine below the Description.
> +        ReviewRequestFieldsHook(self, 'main', [TryField])

Why? do we know what is causing this?

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 4)
> +    revision. Once kicked off, it shows the state of the Try build.

Probably should be "most recent try build"

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 4)
> +    can_record_change_entry = True

So does this mean we can have `can_record_change_entry = True`, and `is_editable = False` and it will still write the change descriptions but dissallow editing?

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 4)
> +    def should_render(self, value):
> +        return isP2RB(self.review_request_details)

Let's make this only render on to the parent review request, not the children.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 4)
> +            'autoland_try': json.dumps(rr.extra_data.get('p2rb.autoland_try', {})),

p2rb.autoland_try should be stored and passed around as javascript int or null, and Python int or None.

Comment 32

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2721/#review2017

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 5)
> +                    activityIndicator.fadeOut("fast");

`RB.setActivityIndicator` can shut off the activity indicator using the default behaviour.

Comment 33

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2703/#review2019

Ship It!

Comment 34

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2705/#review2021

::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 5)
>          'commits': {
>              'source_filenames': ['js/commits.js'],
>          },
>          'rbmozuiautocomplete': {
>              'source_filenames': ['js/ui.rbmozuiautocomplete.js'],
>          },
>          'review': {
>              'source_filenames': ['js/review.js'],
> +        },
> +        'try': {
> +            'source_filenames': ['js/try.js'],
>          }

So all of these bundles end up just being shoved in exactly the same spot, why don't we put them in the same bundle so their minified together in production?

We can still keep them as separate files though, so if we ever need to insert them in different spots we can.

::: pylib/rbmozui/rbmozui/templates/rbmozui/review-scripts-js.html
(Diff revision 5)
>  {% if review_request|isPush %}
>  {%   ext_js_bundle extension "rbmozuiautocomplete" %}
>  {%   ext_js_bundle extension "review" %}
>  {%   ext_js_bundle extension "commits" %}
> +{%   ext_js_bundle extension "try" %}
>  {% endif %}

So, as I said about using the same bundle. I guess we should keep the try bundle separate, because we'll want to not include it on child review requests.

Lets move `{%   ext_js_bundle extension "try" %}` out into its own conditional that makes sure `review_request|isParent` or something.

Comment 35

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2709/#review2023

::: pylib/rbmozui/rbmozui/static/js/common.js
(Diff revision 5)
> +var RBMozUI = {};

Should we maybe be using the built-in Review Board js extension support for this kind of thing? I'm not super familiar with it but are we rolling our own here?

::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 5)
> +        'common': {
> +            'source_filenames': ['js/common.js'],
> +        },

More bundles! heh.

Should this just go in the default bundle? or I guess this could be its own bundle... lets try and cut down though on the number of bundles.

::: pylib/rbmozui/rbmozui/static/js/commits.js
(Diff revision 5)
> -  // End of Backbone model / collection / view definitions.
> +  new CommitListView({model: RBMozUI.rootReviewRequest});

Much nicer :)

::: pylib/rbmozui/rbmozui/static/js/common.js
(Diff revision 5)
> +  RBMozUI.rootReviewRequest = new RB.ReviewRequest({id: rootID});

Should we check the id of gReviewRequest before calling this and if we're on the root just take that value?

Comment 36

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2711/#review2027

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 5)
>  $(document).ready(function() {

didn't really review this file much at all as you suggested.

Comment 37

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2721/#review2029

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 5)
>    var TRY_AUTOLAND = "http://184.169.175.127/autoland/status/1";

Are we planning to have this configurable inside the extension?

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 5)
>    var TREE = "mozreview-gecko";

Just a heads up that we need to be smarter than this, as we have other repos on MozReview. I'm going to assume though this is just temp make it work stuff right now.

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 5)
> +                RBMozUI.rootEditor.setDraftField(TRY_FIELD, value, {
> +                  useExtraData: true,
> +                  jsonFieldName: TRY_FIELD,
> +                  success: function() {
> +                    activityIndicator.fadeOut("fast");
> +                    RBMozUI.rootEditor.on("published", function() {
> +                      window.location.reload();
> +                    })
> +                    RBMozUI.rootEditor.publishDraft();
> +                  },
> +                  error: function(errorObject) {
> +                    console.error(errorObject.errorText);
> +                    // TODO: User facing error message
> +                  }
> +                });

The plan isn't to do this in the future right, we're not going to use the draft at all?

I'm just putting this here anyways to make sure even though you probably address this in a later change.

Comment 38

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2751/#review2031

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 4)
> -  var TRY_AUTOLAND = "http://184.169.175.127/autoland/status/1";
> +  var TRY_AUTOLAND = "api/extensions/mozreview.extension.MozReviewExtension/try-autoland-triggers/";

I feel like the built-in js extensions again can help here. Aren't there ways to have this not hardcoded so that any changes to naming won't break things?

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 4)
> -                  tree: TREE,
> +                  review_request_id: commitID,

This is super misleading. From my quick inspection `review_request_id:` isn't a review request id, it's a 'review identifier'. Also, `commitID` is also a review identifier?

I could be wrong and if so please correct. But the fact alone that we're assinging something called `commitID` to the key of `review_request_id` is problematic.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2701/#review2033

> Why? do we know what is causing this?

At least from my understanding, we _want_ Commits to be at the top. Our choices are to remove the Description field, add the Commit field, and then re-add the Description field, or to do this hack to change the ordering of the fields to put Description right at the bottom.

I think this was more motivated by the fact that 2.0.11 had a bug where if the bottom Field was editable and supplied by an extension, this would cause the page JS to break entirely. This was what mdoglio was experiencing on Day 1 of the work week, and what inspired the bump to 2.0.12 on production.

So, to sum - the hack just above this comment puts Commits at the top, which is probably what we want, and then everything else can go beneath Description, which is... probably fine?

> So does this mean we can have `can_record_change_entry = True`, and `is_editable = False` and it will still write the change descriptions but dissallow editing?

Yes, I believe so.

> p2rb.autoland_try should be stored and passed around as javascript int or null, and Python int or None.

Good call.

Comment 40

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2709/#review2037

> Should we maybe be using the built-in Review Board js extension support for this kind of thing? I'm not super familiar with it but are we rolling our own here?

To be completely honest, I'm not entirely sure how the Review Board JS extension stuff works. I think it's mostly for the Review UI front-ends, but I could be wrong.

> Should we check the id of gReviewRequest before calling this and if we're on the root just take that value?

From the looks of things, gReviewRequest is going away soonish. I'd like to avoid relying on it if we can.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2721/#review2039

> `RB.setActivityIndicator` can shut off the activity indicator using the default behaviour.

I can't find any reference to RB.setActivityIndicator in the codebase. :(

Comment 42

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2721/#review2041

> Are we planning to have this configurable inside the extension?

Yep! This front-end stuff is slowly being invalidated by the backend that is developed in the later patches. Probably best to come back to this later.

> The plan isn't to do this in the future right, we're not going to use the draft at all?
> 
> I'm just putting this here anyways to make sure even though you probably address this in a later change.

Yeah, I think most of this try.js stuff is invalid at this point in the patch series.

Comment 43

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2751/#review2045

> Instead of manually setting the activity indicator, you may want to use `RB.setActivityIndicator` (https://github.com/reviewboard/reviewboard/blob/ea775c5e7b32ee427101e9e1f886b1c0398f9170/reviewboard/static/rb/js/utils/apiUtils.js). I don't know if it will fit your needs completely, so feel free to drop this issue.

This is only in the next RB release (2.5) so never mind.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2749/#review2047

> Do we want to support `@webapi_check_local_site` for generality's sake, or is it not worth worrying about?

Mozilla will never use Localsites, so I think we're fine.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2803/#review2049

> By default won't `x.get('blah')` return `None` if `'blah' not in x` and `None` is "falsy", so are you just doing this for clarity?

Nope - I just forgot how this thing worked. :)

Comment 46

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2749/#review2035

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 4)
> +    autoland_id = models.IntegerField(unique=True)

`models.IntegerField(primary_key=True)`

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 4)
> +    push_sha = models.CharField(max_length=32)

40 characters minimum.

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 4)
> +class AutolandRequest(models.Model):

We're going to need another sha field. One for what we sent autoland, and one for the actual tip that lands on try (The try syntax commit will happen).

We also need to make sure we're happy with this schema before landing, since it'll make development / deployment more painful if we need to change it since we'll be evolving evolutions. Extension evolutions in production are probably not the most tested thing in the world either.

So, you should probably make sure to get review from a few more people involved with this to make sure everyone is happy with the schema.

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 4)
> +    repository_sha = models.CharField(max_length=32, blank=True, default="")

Length is wrong.

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 4)
> +    requestor = models.IntegerField()

I'm assuming this is supposed to be the user who triggered the job's primary key?

I'd prefer something like `user_id` or `submitter_id` to make it more clear it's a poor man's foreign key.

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 4)
> +    class Meta:
> +        app_label = 'mozreview'

Off the top of my head I can't remember exactly how to control the indexing in django models, but I want to make sure you're thinking about it.

I wonder if we should be indexing by any of the shas? or by the review request id?

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 4)
> +class AutolandRequest(models.Model):
> +    autoland_id = models.IntegerField(unique=True)
> +    push_sha = models.CharField(max_length=32)
> +    repository_url = models.CharField(max_length=255, blank=True, default="")
> +    repository_sha = models.CharField(max_length=32, blank=True, default="")
> +    review_request_id = models.IntegerField()
> +    requestor = models.IntegerField()
> +    extra_data = JSONField()

Please add comments describing what these are. At least for some of the more ambiguous ones (push_sha vs repository_sha etc.)

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +from .models import AutolandRequest

`from mozreview.models import AutolandRequest`

or if I'm wrong and it should be directly from the sub-folder

`from mozreview.autoland.models import AutolandRequest`

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +TRY_AUTOLAND_ENDPOINT = "http://184.169.175.127/autoland"
> +TRY_AUTOLAND_DESTINATION = "try"
> +TRY_AUTOLAND_TREE = "mozilla-central"

We should make this configurable in the extension settings.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +    @webapi_login_required
> +    @webapi_response_errors(DOES_NOT_EXIST, INVALID_FORM_DATA,
> +                            NOT_LOGGED_IN, PERMISSION_DENIED)
> +    @webapi_request_fields(

Something else to consider here is local sites. Now that we're adding a model we have to consider if we're going to differentiate between local sites.

Yes, we're not using them right now, but I could see us pushing RB to its limits in the future and maybe making use of them. It'd be nice to have support built-in from the getgo.

We're going to have to make sure we're putting the right decorators on the methods here as well as including information about local site in the model.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +                'description': 'The review request for which to trigger a try build',

Ah okay it actually is a review_request_id! sorry about some of the issues opened in previous requests.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +                'description': 'The TryChooser syntax for the builds we will kick off',

I wonder what validates this and parses it in tryland? would it be worth us verifying before we send to autoland as well?

If it's a python library we can just import and keep up to date, maybe?

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        # Next we need to make sure that the user is either the submitter
> +        # of the review request, or one of the reviewers.
> +        #if request.user.id != rr.submitter.id:
> +        #    return PERMISSION_DENIED

Why is the code commented out? Also the comment above is wrong we only want to allow the submitter (the commented out code appears to do that though)

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        # Get the commit ID from the review request.
> +        if (str(rr.extra_data.get('p2rb', False)).lower() != "true" or
> +            str(rr.extra_data.get('p2rb.is_squashed', False)).lower() != "true"):
> +            return PERMISSION_DENIED

This is neither getting the commit id, or checking anything to do with permissions.

wat?

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        rev = commit_list[-1][0]

we're being really inconsistent with rev vs sha vs commit in all sorts of places. Maybe this can stay as rev but can you consider renaming this with that in mind.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        # TODO: How does Autoland know that we're trustworthy?

I think we need to verify the user's bugzilla cookie, and then send the email along. That could be different from their email registered with a lvl1+ public key though.

Currently you *have* to be lvl1+ to actually create these pushed review requests... so we could just punt on this for a bit. We need to make sure we revisit it though when we start allowing anon pushes.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        if response.status_code != 200:
> +            return WebAPIError(901,
> +                ("Try Autoland returned error code %s" % response.status_code),
> +                http_status=500)  # 500 Internal Server Error

We really want to 500 if something goes wrong? that seems... harsh? Might be a better code out there.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        autoland_request_id = int(response.json().get('request_id', 0)) or None

Can this throw any exceptions?

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +                http_status=500)  # 500 Internal Server Error

again, 500?

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        old_request_id = rr.extra_data.get('p2rb.autoland_try', None)
> +        rr.extra_data['p2rb.autoland_try'] = autoland_request_id
> +        rr.save()

It might be worth commenting about the race condition (we've decided to live with). 

To remind you: It's possible the request that saves their value first here (and gets overwritten by the second) has their changedescription below come second. In that case you'd have the "most recent" try build stats appearing at the top be for a changedescription that has a try build below it (Super rare, not a big deal really).

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        changedesc = ChangeDescription(public=True,
> +                                       text="",
> +                                       rich_text=False)

this didn't fit on one line?

Comment 47

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2803/#review2051

::: pylib/rbmozui/rbmozui/admin_urls.py
(Diff revision 2)
> -from __future__ import unicode_literals
> +from django.conf.urls import patterns

uhhh, what happened to unicode_literals?

Comment 48

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2749/#review2053

> Off the top of my head I can't remember exactly how to control the indexing in django models, but I want to make sure you're thinking about it.
> 
> I wonder if we should be indexing by any of the shas? or by the review request id?

I guess it depends - what's going to be the eventual uses of this table?

> We should make this configurable in the extension settings.

Yeah, this was done in the last commit. Sorry for the switch-a-roo.

> I wonder what validates this and parses it in tryland? would it be worth us verifying before we send to autoland as well?
> 
> If it's a python library we can just import and keep up to date, maybe?

Possibly? That's yet another dependency. Dunno. Might be worth opening a bug about.

> Why is the code commented out? Also the comment above is wrong we only want to allow the submitter (the commented out code appears to do that though)

I've removed this in my latest commit.

> We're going to need another sha field. One for what we sent autoland, and one for the actual tip that lands on try (The try syntax commit will happen).
> 
> We also need to make sure we're happy with this schema before landing, since it'll make development / deployment more painful if we need to change it since we'll be evolving evolutions. Extension evolutions in production are probably not the most tested thing in the world either.
> 
> So, you should probably make sure to get review from a few more people involved with this to make sure everyone is happy with the schema.

Ok, I'll run it by dminor and gps. Thanks!

Comment 49

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2751/#review2043

> I feel like the built-in js extensions again can help here. Aren't there ways to have this not hardcoded so that any changes to naming won't break things?

Yeah, we definitely want to pull this from the MozReview configuration. The whole try.js stuff is kind of a mess at this stage. :/

When I fold this together, maybe we can go through try.js and figure out what's worth keeping.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2751/#review2057

> This is super misleading. From my quick inspection `review_request_id:` isn't a review request id, it's a 'review identifier'. Also, `commitID` is also a review identifier?
> 
> I could be wrong and if so please correct. But the fact alone that we're assinging something called `commitID` to the key of `review_request_id` is problematic.

Yeah, it's supposed to to be the review request ID. I shouldn't be using commitID here - good catch.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.

Pull down this commit:

hg pull review -r 565788242ed5b67aca0dc8570d9b3b265a3164a5

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.

Pull down this commit:

hg pull review -r c07e73e8e6a79cdb6a5922964c01c2ee7d21ecf7

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - [WIP] Bug 1124462 - Test the TryAutolandTriggerResource

Pull down these commits:

hg pull review -r ef4a2a20ae1c28362135fd0d771074d938469773

Comment 54

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/2701/#review2063

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 6)
> +    push_sha = models.CharField(

Can we change it to a more generic push_revision?

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 6)
> +    repository_sha = models.CharField(

change it to repository_revision here?

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 6)
> +

Could it be useful to add a last_modified timestamp?

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 6)
> +        unique=True,

I guess you can omit that the primary key is unique

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +        response = requests.post(TRY_AUTOLAND_ENDPOINT, data=json.dumps({

We should wrap it in a try/catch to catch subclasses of requests.exceptions.RequestException

::: pylib/rbmozui/rbmozui/static/js/try.js
(Diff revision 6)
> +              request = $.ajax({

we should either use var or not use a variable at all

Comment 55

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2701/#review2085

> Could it be useful to add a last_modified timestamp?

Maybe. I'll talk to dminor and gps today and see if we can settle on the schema once and for all.

Comment 56

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/2701/#review2093

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 6)
> +class AutolandRequest(models.Model):
> +    autoland_id = models.IntegerField(
> +        primary_key=True,
> +        unique=True,
> +        help_text=_('The job ID that Autoland returns to us.'))
> +    push_sha = models.CharField(
> +        max_length=40,
> +        help_text=_('The SHA of the commit that Autoland was asked to land.'),
> +        db_index=True)
> +    repository_url = models.CharField(
> +        max_length=255,
> +        blank=True,
> +        default='',
> +        help_text=_('The URL of the repository that Autoland landed on.'))
> +    repository_sha = models.CharField(
> +        max_length=40,
> +        blank=True,
> +        default='',
> +        help_text=_('The SHA of the commit that Autoland landed.'),
> +        db_index=True)
> +    review_request_id = models.IntegerField(
> +        help_text=_('The ID of the review request that Autoland was triggered from.'),
> +        db_index=True)
> +    user_id = models.IntegerField(
> +        help_text=_('The ID of the user that triggered the Autoland job.'),
> +        db_index=True)
> +    extra_data = JSONField(
> +        help_text=_('Meta information about this Autoland job.'))
> +
> +    class Meta:

Please add a comment somewhere about why Django's ORM and foreign keys can't be used because of RB suckitude.

::: pylib/mozreview/mozreview/extension.py
(Diff revision 6)
> +        'autoland_try_url': 'http://184.169.175.127/autoland',

Let's not hardcode this.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +            return AUTOLAND_ERROR, {

We should log response error here.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +        # TODO: How does Autoland know that we're trustworthy?

We should log that we're submitting a Try Autoland request.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +        return 202, {}

I don't believe the use of 202 is justified here.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 6)
> +def isP2RB(review_request):

is_p2rb()

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 6)
> +def isRoot(review_request):

is_root()

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 6)
> +def getRoot(review_request):

get_root()

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 6)
> +def ensureReviewRequest(review_request_details):

ensure_review_request

Comment 57

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/2823/#review2095

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 1)
> +    @CommandArgument('username',
> +                     help='The user to send the request as')
> +    @CommandArgument('password',
> +                     help='The password of the user to send the request as')

I'm trying to keep credentials out of arguments to avoid having to define these arguments on all commands. If you call `exportbzauth` from the .t file, it will export `BUGZILLA_USERNAME` and `BUGZILLA_PASSWORD` in a way that makes the helper scripts happy.

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?

Pull down these commits:

hg pull review -r ab3075b5e6ee6e31a96221f59a9fa48ac5aceffb

Comment 59

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2701/#review2115

> Maybe. I'll talk to dminor and gps today and see if we can settle on the schema once and for all.

We've got an event log table now, so this is resolved.

Comment 60

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?

Pull down these commits:

hg pull review -r 3e04037c3a3286df30d9bd905a860e03e1ab0f88

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?

Pull down these commits:

hg pull review -r 3e04037c3a3286df30d9bd905a860e03e1ab0f88

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?

Pull down these commits:

hg pull review -r 1000f2d7397a6a02b886fd1b916879a1282b9574

Comment 63

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2117

Always look on the bright side of life.
I analyzed your Python changes and found 60 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

::: pylib/mozreview/mozreview/autoland/errors.py
(Diff revision 9)
> +    http_status=502) # 502 Bad Gateway

E261: at least two spaces before inline comment
Separate inline comments by at least two spaces.

An inline comment is a comment on the same line as a statement.  Inline
comments should be separated by at least two spaces from the statement.
They should start with a # and a single space.

Each line of a block comment starts with a # and a single space
(unless it is indented text inside the comment).

Okay: x = x + 1  # Increment x
Okay: x = x + 1    # Increment x
Okay: # Block comment
E261: x = x + 1 # Increment x
E262: x = x + 1  #Increment x
E262: x = x + 1  #  Increment x
E265: #Block comment


::: pylib/mozreview/mozreview/autoland/errors.py
(Diff revision 9)
> +   902,

E121: continuation line under-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreview/mozreview/autoland/errors.py
(Diff revision 9)
> +   http_status=502)

W292: no newline at end of file
Trailing blank lines are superfluous.

Okay: spam(1)
W391: spam(1)\n

However the last line should end with a new line (warning W292).


::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 9)
> +        help_text=_('The revision ID of the commit that Autoland was asked to land.'),

E501: line too long (86 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 9)
> +    # Unfortunately, Review Board extensions can't take advantage of the ForeignKey

E501: line too long (83 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 9)
> +    # ORM magic that Django provides. This is because the extension loading mechanism

E501: line too long (85 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 9)
> +        help_text=_('The ID of the review request that Autoland was triggered from.'),

E501: line too long (86 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 9)
> +    PROBLEM   = 'P'

E221: multiple spaces before operator
Avoid extraneous whitespace around an operator.

Okay: a = 12 + 3
E221: a = 4  + 5
E222: a = 4 +  5
E223: a = 4\t+ 5
E224: a = 4 +\t5


::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 9)
> +    SERVED    = 'S'

E221: multiple spaces before operator
Avoid extraneous whitespace around an operator.

Okay: a = 12 + 3
E221: a = 4  + 5
E222: a = 4 +  5
E223: a = 4\t+ 5
E224: a = 4 +\t5


::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 9)
> +        app_label = 'mozreview'

W292: no newline at end of file
Trailing blank lines are superfluous.

Okay: spam(1)
W391: spam(1)\n

However the last line should end with a new line (warning W292).


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +from django.db import transaction

F401: 'transaction' imported but unused

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +from djblets.webapi.errors import (DOES_NOT_EXIST,

F401: 'WebAPIError' imported but unused

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +from reviewboard.reviews.models import ReviewRequest, ReviewRequestDraft

F401: 'ReviewRequestDraft' imported but unused

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +class TryAutolandTriggerResource(WebAPIResource):

E302: expected 2 blank lines, found 1
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            'description': 'The Autoland request ID that Autoland gave back to us.',

E501: line too long (84 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            'description': 'The revision of what got pushed for Autoland to land.',

E501: line too long (83 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            'description': 'The repository that Autoland was asked to land on.',

E501: line too long (80 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            'description': 'The revision of what Autoland landed on the repository.',

E501: line too long (85 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            'description': 'The review request associated with this Autoland request.',

E501: line too long (87 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                'description': 'The review request for which to trigger a try build',

E501: line too long (85 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                'description': 'The TryChooser syntax for the builds we will kick off',

E501: line too long (87 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                'description': 'For testing only. If the MozReview extension is in testing mode, '

E501: line too long (98 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                               'this skips the request to Try Autoland and just uses this request '

E501: line too long (99 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        if (six.text_type(rr.extra_data.get('p2rb', False)).lower() != "true" or

E501: line too long (80 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            six.text_type(rr.extra_data.get('p2rb.is_squashed', False)).lower() != "true"):

E129: visually indented line with same indent as next logical line
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            six.text_type(rr.extra_data.get('p2rb.is_squashed', False)).lower() != "true"):

E501: line too long (91 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            logging.error('Failing because either not p2rb or not p2rb.is_squashed')

E501: line too long (84 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        ext = get_extension_manager().get_enabled_extension('mozreview.extension.MozReviewExtension')

E501: line too long (101 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +            logging.info('Submitting a request to Autoland for review request ID %s '

E501: line too long (85 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                         'for revision %s ' % (review_request_id, last_revision))

E501: line too long (81 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                logging.error('We hit a RequestException when submitting a request '

E501: line too long (84 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                logging.error('We timed out when submitting a request to Autoland')

E501: line too long (83 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                    status_code: response.status_code,

F821: undefined name 'status_code'

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                autoland_request_id = int(response.json().get('request_id', 0)) or None

E501: line too long (87 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                        status_code: response.status_code,

F821: undefined name 'status_code'

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                        request_id: autoland_request_id,

F821: undefined name 'request_id'

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        event_log_entry = AutolandEventLogEntry(status=AutolandEventLogEntry.REQUESTED,

F841: local variable 'event_log_entry' is assigned to but never used

Indicates that a variable has been explicity assigned to but not actually
used.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        event_log_entry = AutolandEventLogEntry(status=AutolandEventLogEntry.REQUESTED,

E501: line too long (87 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +                                                autoland_request=autoland_request)

E501: line too long (82 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        # There's possibly a race condition here with multiple web-heads. If two requests

E501: line too long (89 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        # come in at the same time to this endpoint, request A saves their value first

E501: line too long (86 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        # here (and gets overwritten by the second B) has their changedescription below

E501: line too long (87 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        # come second. In that case you'd have the "most recent" try build stats appearing

E501: line too long (90 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        # at the top be for a changedescription that has a try build below it (Super rare,

E501: line too long (90 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 9)
> +        changedesc.record_field_change('extra_data.p2rb.autoland_try', old_request_id,

E501: line too long (86 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/extension.py
(Diff revision 9)
> +      try_autoland_trigger_resource,

E121: continuation line under-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreview/mozreview/extension.py
(Diff revision 9)
> +        register_resource_for_model(AutolandRequest, try_autoland_trigger_resource)

E501: line too long (83 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/models.py
(Diff revision 9)
> +from mozreview.autoland.models import AutolandRequest

F401: 'AutolandRequest' imported but unused

::: pylib/mozreview/mozreview/models.py
(Diff revision 9)
> +from mozreview.autoland.models import AutolandRequest

W292: no newline at end of file
Trailing blank lines are superfluous.

Okay: spam(1)
W391: spam(1)\n

However the last line should end with a new line (warning W292).


::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 9)
> +        # The above hack forced Commits at the top, but the rest of these fields

E501: line too long (80 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 9)
> +            str(review_request.extra_data.get('p2rb.is_squashed', False)).lower() == "true")

E501: line too long (92 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 9)
> +                          'commit id %s because it does not appear to exist, or '

E501: line too long (81 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 9)
> +        ext = get_extension_manager().get_enabled_extension('rbmozui.extension.RBMozUI')

E501: line too long (88 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 9)
> +            'autoland_try': json.dumps(rr.extra_data.get('p2rb.autoland_try', None)),

E501: line too long (85 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 9)
> +            'commit_id': "abcdefg1238d9s8df7s7df8sa7df7sd8df", # TODO

E261: at least two spaces before inline comment
Separate inline comments by at least two spaces.

An inline comment is a comment on the same line as a statement.  Inline
comments should be separated by at least two spaces from the statement.
They should start with a # and a single space.

Each line of a block comment starts with a # and a single space
(unless it is indented text inside the comment).

Okay: x = x + 1  # Increment x
Okay: x = x + 1    # Increment x
Okay: # Block comment
E261: x = x + 1 # Increment x
E262: x = x + 1  #Increment x
E262: x = x + 1  #  Increment x
E265: #Block comment


::: pylib/rbmozui/rbmozui/forms.py
(Diff revision 9)
> +from django.utils.translation import ugettext as _

F401: '_' imported but unused

::: pylib/rbmozui/rbmozui/forms.py
(Diff revision 9)
> +        help_text="This exposes the field in a push-based review request for scheduling Try Autoland jobs",

E501: line too long (107 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

Comment 64

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2119

Always look on the bright side of life.
I analyzed your Python changes and found 9 errors.

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 4)
> +        description='Pass some values to the try-autoland-trigger WebAPI resource.')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 4)
> +        description='Pass some values to the try-autoland-trigger WebAPI resource.')

E501: line too long (84 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 4)
> +                     help='The review request ID to pass to the WebAPI resource')

E501: line too long (81 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 4)
> +        ext = root.get_extension(extension_name="mozreview.extension.MozReviewExtension")

E501: line too long (89 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 4)
> +            ext.get_try_autoland_triggers().create(review_request_id=review_request_id,

E501: line too long (87 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 4)
> +                                                   autoland_request_id_for_testing=autoland_request_id)

E501: line too long (103 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 4)
> +        ext = root.get_extension(extension_name="mozreview.extension.MozReviewExtension")

E501: line too long (89 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

Comment 65

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?

Pull down these commits:

hg pull review -r 5da37573fe42086f2069fde7da665f3517ee7b24

Comment 66

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2129

Always look on the bright side of life.

I analyzed your Python changes and found 7 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 10)
> +                'description': 'The TryChooser syntax for the builds that will '

E501: line too long (80 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 10)
> +                auth=('autoland', 'autoland'))

E122: continuation line missing indentation or outdented
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 10)
> +        AutolandEventLogEntry.objects.create(
> +             status=AutolandEventLogEntry.REQUESTED,

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 10)
> -        main_fieldset.field_classes.insert(0, main_fieldset.field_classes.pop())
> +        main_fieldset.field_classes.insert(0,
> +            main_fieldset.field_classes.pop())

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 10)
> +import json

F401: 'json' imported but unused

Comment 67

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2131

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 68

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?

Pull down these commits:

hg pull review -r 467ded9827e7d30558373d528cfaf65c68ead5ac

Comment 69

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2135

Always look on the bright side of life.

I analyzed your Python changes and found 3 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 11)
> +                   auth=(autoland_user, autoland_password))

E121: continuation line under-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)

Comment 70

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2137

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 71

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2675/#review2165

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 20)
> +from __future__ import unicode_literals

We import this twice.

Comment 72

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/2885 - mozreview: Add API endpoint for Autoland status updates.

Pull down these commits:

hg pull review -r b2366aeb5114ffaa52ae84e34e15ce3cacf82d65

Comment 73

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2187

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

::: pylib/mozreview/mozreview/forms.py
(Diff revision 12)
>      pulse_password = forms.CharField(required=False, widget=forms.PasswordInput)

E501: line too long (80 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 12)
>          TemplateHook(self, 'base-scripts-post', 'rbmozui/review-scripts-js.html',

E501: line too long (81 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

Comment 74

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2189

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 7)
>          c = RBClient('http://localhost:%s/' % port, username=username,
>                  password=password)

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 7)
>      @Command('start', category='reviewboard',
>          description='Start a Review Board HTTP server.')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)

Comment 75

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2885/#review2191

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/extension.py

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 1)
> +            last_event = self.event_log_entries.order_by('-event_time').all()[0]

E501: line too long (80 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 1)
> +import datetime

F401: 'datetime' imported but unused

Comment 76

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2885/#review2195

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 1)
>          app_label = 'mozreview'

You can add the following

    ordering = ['-event_time']
    get_latest_by = 'event_time'

and then you can use `.get_latest()` like I mentioned in the other comment.

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 1)
> +            last_event = self.event_log_entries.order_by('-event_time').all()[0]

You can use `last_event = self.event_log_entries.get_latest()` if you look at my other comment.

Comment 77

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2885/#review2197

> You can use `last_event = self.event_log_entries.get_latest()` if you look at my other comment.

If you do, you will have to catch `AutoLandEventEntry.DoesNotExist` instead of `IndexError`

Comment 78

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2885/#review2199

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 1)
> +            'description': 'Wether this push landed ro not'

`ro` -> `or`

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 1)
> +            'description': ('Either an error message '
> +                            'or the revision of the push landed')

You don't need parens around multiline strings. Same elsewhere.

Comment 79

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/2885 - mozreview: Add API endpoint for Autoland status updates.

Pull down these commits:

hg pull review -r 154b3921159e223fbcbb8e6d8810a0c64d0b3b86

Comment 80

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2205

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

Comment 81

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2207

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 82

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2885/#review2209

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/extension.py

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 2)
> +            last_event = self.event_log_entries.order_by('-event_time').all()[0]

E501: line too long (80 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 2)
> +import datetime

F401: 'datetime' imported but unused

Comment 83

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/2885 - mozreview: Add API endpoint for Autoland status updates.

Pull down these commits:

hg pull review -r f6c1f588a47b4af6727df86d8e671f95ba2ffdd8

Comment 84

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2229

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

Comment 85

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2231

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 86

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2885/#review2233

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/extension.py

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 3)
> +        kwargs = {

F841: local variable 'kwargs' is assigned to but never used

Indicates that a variable has been explicity assigned to but not actually
used.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 3)
> +                error: e,

F821: undefined name 'error'

Comment 87

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/2885 - mozreview: Add API endpoint for Autoland status updates.

Pull down these commits:

hg pull review -r a6e2ba2385f65b553c6616e1798ef52484d987a3

Comment 88

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2241

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

Comment 89

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2243

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 90

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2885/#review2245

Always look on the bright side of life.

I analyzed your Python changes and found 3 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/extension.py
  testing/vcttesting/reviewboard/mach_commands.py

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        kwargs = {

F841: local variable 'kwargs' is assigned to but never used

Indicates that a variable has been explicity assigned to but not actually
used.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +                logging.debug('Type of %s is %s' % (field_name, type(fields[field_name])))

E501: line too long (90 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 4)
> +        update_queryset = AutolandRequest.objects.filter(pk=fields['request_id'])

E501: line too long (81 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

Comment 91

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/2885 - mozreview: Add API endpoint for Autoland status updates.

Pull down these commits:

hg pull review -r 3ec935c475774d5e9dc132522440adbccdac9a4f

Comment 92

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2247

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

Comment 93

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2249

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 94

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2885/#review2251

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/extension.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 95

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/2885 - mozreview: Add API endpoint for Autoland status updates.
/r/2965 - rbmozui: Render changedescriptions for autoland requests.

Pull down these commits:

hg pull review -r 92740fbf37bb57f52ab24b232e7921f33683eb86

Comment 96

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2253

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

Comment 97

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2255

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 98

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2885/#review2257

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/extension.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 99

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2965/#review2259

Always look on the bright side of life.

I analyzed your Python changes and found 3 errors.

The following files were examined:

  pylib/rbmozui/rbmozui/fields.py

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 1)
> +            logging.error('An unknown autoland_id was detected: %s' %
> +                info['new'][0])

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 1)
> +            return  '<a href="%s">%s</a>' % (url, url)

E271: multiple spaces after keyword
Avoid extraneous whitespace around keywords.

Okay: True and False
E271: True and  False
E272: True  and False
E273: True and\tFalse
E274: True\tand False


::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 1)
> +        return _waiting_txt

F821: undefined name '_waiting_txt'

Comment 100

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8551427 [details]
MozReview Request: bz://1109218/mconley

/r/2701 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/2823 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/2885 - mozreview: Add API endpoint for Autoland status updates.
/r/2965 - rbmozui: Render changedescriptions for autoland requests.

Pull down these commits:

hg pull review -r ec76f92197ab34e84f9d5a795c478d283180746b

Comment 101

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2701/#review2283

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/autoland/__init__.py
  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/errors.py
  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/forms.py
  pylib/mozreview/mozreview/models.py
  pylib/rbmozui/rbmozui/admin_urls.py
  pylib/rbmozui/rbmozui/extension.py
  pylib/rbmozui/rbmozui/fields.py
  pylib/rbmozui/rbmozui/forms.py

Comment 102

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2823/#review2285

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/__init__.py
  testing/vcttesting/reviewboard/mach_commands.py

Comment 103

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2885/#review2287

Always look on the bright side of life.

I analyzed your Python changes and found 3 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/errors.py
  pylib/mozreview/mozreview/autoland/models.py
  pylib/mozreview/mozreview/autoland/resources.py
  pylib/mozreview/mozreview/extension.py
  testing/vcttesting/reviewboard/mach_commands.py

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +        # TODO - remove this before landing. Just for local testing on mconley's machine

E501: line too long (88 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +        #return "http://192.168.59.3:60353/api/extensions/mozreview.extension.MozReviewExtension/autoland-request-updates/"

E265: block comment should start with '# '
Separate inline comments by at least two spaces.

An inline comment is a comment on the same line as a statement.  Inline
comments should be separated by at least two spaces from the statement.
They should start with a # and a single space.

Each line of a block comment starts with a # and a single space
(unless it is indented text inside the comment).

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +        #return "http://192.168.59.3:60353/api/extensions/mozreview.extension.MozReviewExtension/autoland-request-updates/"

E501: line too long (123 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

Comment 104

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2965/#review2289

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  pylib/mozreview/mozreview/autoland/models.py
  pylib/rbmozui/rbmozui/fields.py

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 2)
> +            logging.error('An unknown autoland_id was detected: %s' %
> +                info['new'][0])

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 2)
> +            return  '<a href="%s">%s</a>' % (url, url)

E271: multiple spaces after keyword
Avoid extraneous whitespace around keywords.

Comment 105

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/2701/#review2295

::: pylib/mozreview/mozreview/errors.py
(Diff revision 13)
> +    800,

I'm just curious: where do these numeric error codes come from? Are we inventing them as we go? Or, are they coming from somewhere else? If coming from somewhere else, are there constants we can use so we don't have magic numbers?

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 13)
> +        primary_key=True,
> +        help_text=_('The job ID that Autoland returns to us.'))

Does Django's ORM not imply primary_key => autoincrement? Typically ORMs do. I would expect this to fail with sparse integer ranges.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 13)
> +        if not rr.is_mutable_by(request.user):
> +            return PERMISSION_DENIED

We'll eventually want other people (like Sheriffs) to trigger Autoland foo. What's the story for this?

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 13)
> +        testing = ext.settings.get('testing', False)
> +
> +        if testing:
> +            logging.info('In testing mode - storing autoland request id %s'
> +                         % autoland_request_id_for_testing)
> +            autoland_request_id = autoland_request_id_for_testing

`testing` is a very generic setting. I can imagine other features may want similar flags.

Either create an autoland-specific setting or use a special value for the autoland host (like `testing`).

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 13)
> +            autoland_host = ext.settings.get('autoland_host')
> +            if not autoland_host:
> +                return BAD_AUTOLAND_URL

Let's rename the setting to `autoland_url` since that's what it is. `host` to me implies a hostname, without a scheme, port, or path component.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 13)
> +                    'tree': TRY_AUTOLAND_TREE,
> +                    'rev': last_revision,
> +                    'destination': TRY_AUTOLAND_DESTINATION,
> +                    'trysyntax': try_syntax,

So this endpoint is hardcoded for Try. We'll eventually want to open Autoland for other trees. What's your plan there? New API? Break backwards compatibility?

I think I'd be fine breaking backwards compatibility. Just as long as the only consumer is stuff on the server (which includes the web interface).

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 13)
> +        autoland_request = AutolandRequest.objects.create(
> +            autoland_id=autoland_request_id,
> +            push_revision=last_revision,
> +            review_request_id=rr.id,
> +            user_id=request.user.id,
> +            extra_data=json.dumps({
> +                'try_syntax': try_syntax
> +            })
> +        )
> +
> +        AutolandEventLogEntry.objects.create(
> +            status=AutolandEventLogEntry.REQUESTED,
> +            autoland_request=autoland_request)
> +
> +        # There's possibly a race condition here with multiple web-heads. If
> +        # two requests come in at the same time to this endpoint, the request
> +        # that saves their value first here will get overwritten by the second
> +        # but the first request will have their changedescription come below
> +        # the second. In that case you'd have the "most recent" try build stats
> +        # appearing at the top be for a changedescription that has a different
> +        # try build below it (Super rare, not a big deal really).
> +        old_request_id = rr.extra_data.get('p2rb.autoland_try', None)
> +        rr.extra_data['p2rb.autoland_try'] = autoland_request_id
> +        rr.save()
> +
> +        # In order to display the fact that a build was kicked off in the UI,
> +        # we construct a change description that our TryField can render.
> +        changedesc = ChangeDescription(public=True, text='', rich_text=False)
> +        changedesc.record_field_change('extra_data.p2rb.autoland_try',
> +                                       old_request_id, autoland_request_id)
> +        changedesc.save()
> +        rr.changedescs.add(changedesc)

We have ~4 database mutations here that are all related. Are these all executed inside a transaction? Should they be?

I didn't look at the web foo in much detail.

Comment 106

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/2823/#review2297

::: pylib/mozreview/mozreview/autoland/tests/test-try-autoland-trigger-webapi-submitter.t
(Diff revision 8)
> +  $ rbmanage stop rbserver
> +  $ dockercontrol stop-bmo rb-test-try-autoland-trigger

This should now be `mozreview stop`

::: pylib/mozreview/mozreview/autoland/tests/test-try-autoland-trigger-webapi-submitter.t
(Diff revision 8)
> +  $ commonenv rb-test-try-autoland-trigger

You don't need the argument to `commonenv` any more.

::: pylib/mozreview/mozreview/autoland/tests/test-try-autoland-trigger-webapi-submitter.t
(Diff revision 8)
> +  $ hg --config bugzilla.username=[e-mail] push http://localhost:$HGPORT/ > /dev/null

You should be able to just `hg --config ... push` without the remote URL.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 8)
> +    @CommandArgument('--autoland-request-id', default=1,

Default the request id to 1 seems dangerous.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 8)
> +            print e

Let's throw a `return ` in here just to appease the shell gods.

Comment 107

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/2885/#review2299

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +                    'endpoint': endpoint,

I know this isn't your API, but `endpoint` is a very ambiguous name. This should be `pingback_url` or similar.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> -        changedesc.record_field_change('extra_data.p2rb.autoland_try',
> +        changedesc.record_field_change('p2rb.autoland_try',

This feels like it should be part of the earier commit.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +        'result': {
> +            'type': six.text_type,
> +            'description': 'Either an error message or the revision of the '
> +                           'push landed'

Overloading fields for multiple uses is frowned upon. You should use separate fields to convey separate meanings.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +            'description': 'The revision of what got pushed for Autoland to'
> +                           'grab'

I do not understand what this means.

::: pylib/mozreview/mozreview/autoland/resources.py
(Diff revision 6)
> +        try:
> +            AutolandRequest.objects.get(pk=fields['request_id'])
> +        except AutolandRequest.DoesNotExist:
> +            return INVALID_FORM_DATA

Is there any way to denote that the request id didn't exist? That seems like something a consumer may want to differentiate on.

::: pylib/mozreview/mozreview/autoland/tests/test-autoland-request-update-webapi.t
(Diff revision 6)
> +  $ commonenv rb-test-try-autoland-trigger

commonenv

::: pylib/mozreview/mozreview/autoland/tests/test-autoland-request-update-webapi.t
(Diff revision 6)
> +  $ hg --config bugzilla.username=[e-mail] push http://localhost:$HGPORT/ > /dev/null

Remove the URL.

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 6)
> +            last_evt = self.event_log_entries.order_by('-event_time').all()[0]

Relying on time based ordering scares me a bit. Clocks can and do get skewed and report bad values. If you care about this being accurate, I would recommend putting an autoincrementing primary key on the log entries table and selecting the largest value.

There could also be an issue with granularity. If we only have 1s granularity and we get 2 updates in the same second (which could happen during testing), what happens?

::: pylib/mozreview/mozreview/autoland/tests/test-autoland-request-update-webapi.t
(Diff revision 6)
> +  $ rbmanage stop rbserver
> +  $ dockercontrol stop-bmo rb-test-try-autoland-trigger

mozreview stop

Comment 108

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/2965/#review2301

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 2)
> +    _job_url = 'https://treeherder.mozilla.org/#/jobs?repo=try&revision=%s'

The `try` push of this URL feels like something that should be derived from a database column or something. Or did we decide to defer this to a future migration?

Comment 109

[Mike Conley (:mconley) (:⚙️)]

Hey mdoglio,

gps has some really good questions / review comments in here - but I'm about to turn back into a pumpkin and work on e10s stuff again. Are you good to drive this one into the ground?

Comment 110

[Dan Minor [:dminor]]

(In reply to Gregory Szorc [:gps] from comment #107)
> https://reviewboard.mozilla.org/r/2885/#review2299
> 
> ::: pylib/mozreview/mozreview/autoland/resources.py
> (Diff revision 6)
> > +                    'endpoint': endpoint,
> 
> I know this isn't your API, but `endpoint` is a very ambiguous name. This
> should be `pingback_url` or similar.

I have no problems renaming this (or anything else) in autoland for greater clarity. It's best if these things match between autoland and mozreview.

Comment 111

[Mauro Doglio [:mdoglio]]

:mconley yeah I'll work on it on either Tuesday or Wednesday this week.

Comment 112

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2885/#review2555

> Is there any way to denote that the request id didn't exist? That seems like something a consumer may want to differentiate on.

You can use the `DOES_NOT_EXIST` error code for this.

Comment 113

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/2965/#review2557

::: pylib/mozreview/mozreview/autoland/models.py
(Diff revision 2)
> +            last_evt = self.event_log_entries.order_by('-event_time').all()[0]

You should be able to use `.first()` instead of `.all()[0]` here.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 2)
> +            # removed and not replaced with a new one, which would be very
> +            # strange.

If this is an exceptional case, should an error be logged here?

Comment 114

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: bz://1109218/mdoglio

/r/3097 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/3099 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/3101 - mozreview: Add API endpoint for Autoland status updates.
/r/3103 - rbmozui: Render changedescriptions for autoland requests.
/r/3105 - Rename autoland_host setting to autoland_url
/r/3637 - Rename testing setting to autoland_testing
/r/3639 - Rename endpoint attribute to pingback_url
/r/3641 - Use max id instead of event_time to retrieve the last AutolandEventLogEntry
/r/3643 - Use separate fields for result and error_msg on AutolandRequestUpdateResource
/r/3645 - Update description of rev field on AutolandRequestUpdateResource
/r/3647 - Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)

Pull down these commits:

hg pull review -r 5075dc0f02a99eb8fa727b671045ebf4c17f5294

Comment 115

[Mauro Doglio [:mdoglio]]

Comment on attachment 8562006 [details]
MozReview Request: bz://1109218/mdoglio

/r/3097 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/3099 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/3101 - mozreview: Add API endpoint for Autoland status updates.
/r/3103 - rbmozui: Render changedescriptions for autoland requests.
/r/3105 - Rename autoland_host setting to autoland_url
/r/3637 - Rename testing setting to autoland_testing
/r/3639 - Rename endpoint attribute to pingback_url
/r/3641 - Use max id instead of event_time to retrieve the last AutolandEventLogEntry
/r/3643 - Use separate fields for result and error_msg on AutolandRequestUpdateResource
/r/3645 - Update description of rev field on AutolandRequestUpdateResource
/r/3647 - Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)
/r/3649 - use .first() instead of .all()[0] (bug 1109218)

Pull down these commits:

hg pull review -r 5e5311cebdeb65a43b30aced603683af437a1cb3

Comment 116

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/3639/#review2843

I've pushed the corresponding changes to Autoland in https://hg.mozilla.org/hgcustom/version-control-tools/rev/a1cb9b5dd2e8

Comment 117

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/3643/#review2845

Corresponding autoland change: https://hg.mozilla.org/hgcustom/version-control-tools/rev/b1fbfc11f837

Comment 118

[Mauro Doglio [:mdoglio]]

Comment on attachment 8562006 [details]
MozReview Request: bz://1109218/mdoglio

/r/3097 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/3099 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/3101 - mozreview: Add API endpoint for Autoland status updates.
/r/3103 - rbmozui: Render changedescriptions for autoland requests.
/r/3105 - Rename autoland_host setting to autoland_url
/r/3637 - Rename testing setting to autoland_testing
/r/3639 - Rename endpoint attribute to pingback_url
/r/3641 - Use max id instead of event_time to retrieve the last AutolandEventLogEntry
/r/3643 - Use separate fields for result and error_msg on AutolandRequestUpdateResource
/r/3645 - Update description of rev field on AutolandRequestUpdateResource
/r/3647 - Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)
/r/3649 - use .first() instead of .all()[0] (bug 1109218)

Pull down these commits:

hg pull review -r 5e5311cebdeb65a43b30aced603683af437a1cb3

Comment 119

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/3649/#review2853

Your commit message should say `.last()` instead of `.first()` because thats what you're actually using in the code.

Comment 120

[Mauro Doglio [:mdoglio]]

Comment on attachment 8562006 [details]
MozReview Request: bz://1109218/mdoglio

/r/3097 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/3099 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/3101 - mozreview: Add API endpoint for Autoland status updates.
/r/3103 - rbmozui: Render changedescriptions for autoland requests.
/r/3105 - Rename autoland_host setting to autoland_url
/r/3637 - Rename testing setting to autoland_testing
/r/3639 - Rename endpoint attribute to pingback_url
/r/3641 - Use max id instead of event_time to retrieve the last AutolandEventLogEntry
/r/3643 - Use separate fields for result and error_msg on AutolandRequestUpdateResource
/r/3645 - Update description of rev field on AutolandRequestUpdateResource
/r/3647 - Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)
/r/3649 - use .last() instead of .all()[0] (bug 1109218)

Pull down these commits:

hg pull review -r 48ae7dafc5869898052347c5dce593e086ebfde7

Comment 121

[Dan Minor [:dminor]]

I'm hitting a few tests that fail due to changes in the diff id, like the following:

--- /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-commits-added.t
+++ /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-commits-added.t.err
@@ -108,7 +108,7 @@
       p2rb.is_squashed: true
       p2rb.unpublished_rids: '[]'
     diffs:
-    - id: 4
+    - id: 6
       revision: 1
       base_commit_id: null
       patch:

I'm having trouble determining if this is a significant change or not. I've tried adding asserts in various places and I'm not seeing extra diffs being created. Because the review ids match the expected results, I don't think this is due to extra reviews being created.

Comment 122

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3099/#review2939

This is good except for the nits.

::: pylib/mozreview/mozreview/autoland/tests/test-try-autoland-trigger-webapi-submitter.t
(Diff revision 1)
> +  $ commonenv rb-test-try-autoland-trigger

You don't need an argument to `commonenv` any more.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 1)
> +        except:

except Exception:

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 1)
> +            print e

return 1

Comment 123

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3103/#review2941

You should get review from one of the RB gurus, but this LGTM.

Comment 124

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3105/#review2943

Ship It!

Comment 125

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3637/#review2945

Ship It!

Comment 126

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3641/#review2947

Empty file?

Comment 127

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3643/#review2949

Ship It!

Comment 128

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3645/#review2951

Ship It!

Comment 129

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/3641/#review2957

It shouldn't be empty, this is my output of hg export -r d260bbe4 
http://pastebin.mozilla.org/8673890
I think I have the same problem on c3dbffd1

Comment 130

[Mauro Doglio [:mdoglio]]

Comment on attachment 8562006 [details]
MozReview Request: bz://1109218/mdoglio

/r/3097 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/3099 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/3101 - mozreview: Add API endpoint for Autoland status updates.
/r/3103 - rbmozui: Render changedescriptions for autoland requests.
/r/3105 - Rename autoland_host setting to autoland_url
/r/3637 - Rename testing setting to autoland_testing
/r/3639 - Rename endpoint attribute to pingback_url
/r/3641 - Use max id instead of event_time to retrieve the last AutolandEventLogEntry
/r/3643 - Use separate fields for result and error_msg on AutolandRequestUpdateResource
/r/3645 - Update description of rev field on AutolandRequestUpdateResource
/r/3647 - Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)
/r/3649 - use .last() instead of .all()[0] (bug 1109218)

Pull down these commits:

hg pull review -r 48ae7dafc5869898052347c5dce593e086ebfde7

Comment 131

[Mauro Doglio [:mdoglio]]

Comment on attachment 8562006 [details]
MozReview Request: bz://1109218/mdoglio

/r/3097 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/3099 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/3101 - mozreview: Add API endpoint for Autoland status updates.
/r/3103 - rbmozui: Render changedescriptions for autoland requests.
/r/3105 - Rename autoland_host setting to autoland_url
/r/3637 - Rename testing setting to autoland_testing
/r/3639 - Rename endpoint attribute to pingback_url
/r/3641 - Use max id instead of event_time to retrieve the last AutolandEventLogEntry
/r/3643 - Use separate fields for result and error_msg on AutolandRequestUpdateResource
/r/3645 - Update description of rev field on AutolandRequestUpdateResource
/r/3647 - Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)
/r/3649 - use .last() instead of .all()[0] (bug 1109218)

Pull down these commits:

hg pull review -r c194e1258fc9748de046455b95cb00bf08548c86

Comment 132

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/3641/#review2959

And those changes magically appeared when I updated the review. Note that I didn't touch this revision before pushing.

Comment 133

[Dan Minor [:dminor]]

(In reply to Dan Minor [:dminor] from comment #121)
> I'm hitting a few tests that fail due to changes in the diff id, like the
> following:
> 
> ---
> /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-commits-
> added.t
> +++
> /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-commits-
> added.t.err
> @@ -108,7 +108,7 @@
>        p2rb.is_squashed: true
>        p2rb.unpublished_rids: '[]'
>      diffs:
> -    - id: 4
> +    - id: 6
>        revision: 1
>        base_commit_id: null
>        patch:
> 
> I'm having trouble determining if this is a significant change or not. I've
> tried adding asserts in various places and I'm not seeing extra diffs being
> created. Because the review ids match the expected results, I don't think
> this is due to extra reviews being created.

Oops, wrong bug :/

Comment 134

[Mauro Doglio [:mdoglio]]

Comment on attachment 8562006 [details]
MozReview Request: bz://1109218/mdoglio

/r/3097 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/3099 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/3101 - mozreview: Add API endpoint for Autoland status updates.
/r/3103 - rbmozui: Render changedescriptions for autoland requests.
/r/3105 - Rename autoland_host setting to autoland_url
/r/3637 - Rename testing setting to autoland_testing
/r/3639 - Rename endpoint attribute to pingback_url
/r/3641 - Use max id instead of event_time to retrieve the last AutolandEventLogEntry
/r/3643 - Use separate fields for result and error_msg on AutolandRequestUpdateResource
/r/3645 - Update description of rev field on AutolandRequestUpdateResource
/r/3647 - Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)
/r/3649 - use .last() instead of .all()[0] (bug 1109218)
/r/3739 - Embed treeherder iframe to show try results (Bug 1109218)

Pull down these commits:

hg pull review -r c7d9adfcfa92ffa5b80c5c8b3df30074e9da8259

Comment 135

[Mauro Doglio [:mdoglio]]

Comment on attachment 8562006 [details]
MozReview Request: bz://1109218/mdoglio

/r/3097 - rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.
/r/3099 - mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?
/r/3101 - mozreview: Add API endpoint for Autoland status updates.
/r/3103 - rbmozui: Render changedescriptions for autoland requests.
/r/3105 - Rename autoland_host setting to autoland_url
/r/3637 - Rename testing setting to autoland_testing
/r/3639 - Rename endpoint attribute to pingback_url
/r/3641 - Use max id instead of event_time to retrieve the last AutolandEventLogEntry
/r/3643 - Use separate fields for result and error_msg on AutolandRequestUpdateResource
/r/3645 - Update description of rev field on AutolandRequestUpdateResource
/r/3647 - Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)
/r/3649 - use .last() instead of .all()[0] (bug 1109218)
/r/3739 - Embed treeherder iframe to show try results (Bug 1109218)

Pull down these commits:

hg pull review -r c7d9adfcfa92ffa5b80c5c8b3df30074e9da8259

Comment 136

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/3739/#review3107

This looks good to me.

Comment 137

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/3639/#review3137

Ship It!

Comment 138

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/3643/#review3139

Ship It!

Comment 139

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/2675/#review3311

::: pylib/mozreview/mozreview/extension.py
(Diff revision 27)
> +    resources = [
> +        autoland_request_update_resource,
> +        try_autoland_trigger_resource,
> +    ]

You are overwriting 'resources' here.

Also, am I crazy, or did this land already with no r+s??

Comment 140

[Mark Côté [:mcote]]

What is going on with this bug?  There are duplicated patches in the two reviews, and *something* landed already, related to this bug, with the issue I mentioned in comment #139:

http://hg.mozilla.org/hgcustom/version-control-tools/diff/0521758ed49b/pylib/mozreview/mozreview/extension.py#l1.41

Comment 141

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2675/#review3315

> You are overwriting 'resources' here.
> 
> Also, am I crazy, or did this land already with no r+s??

mdoglio took this over in https://reviewboard.mozilla.org/r/3095/.

Comment 142

[Steven MacLeod [:smacleod]]

These changes didn't get a proper final review before landing - we should definitely have someone look through the commits that landed and give it a review.

Mauro, could you comment with the links to which commits for this stuff landed and needinfo me so I can review it?

Comment 143

[Mauro Doglio [:mdoglio]]

Here is a link to that push
https://hg.mozilla.org/hgcustom/version-control-tools/pushloghtml?changeset=2b3ead5a152d

Comment 144

[Steven MacLeod [:smacleod]]

Diff for this review from "hg diff -r 453f3494d33c:2b3ead5a152d"

I only reviewed the commits themselves and not the current state of the code so
some of these comments might be stale. Also, I didn't really review the changes
to the front-end commit list stuff since we're tearing that out and replacing
with server-side rendering anyways.

General comment about all of the WebAPIError's introduced in the commits - All
error codes should be greater than 1000 (which RB core guarantees it will not
use itself). Also, I'm thinking it might just be best to stick them all in the
same file (including ones introduced by unrelated commits here). That way
finding a particular error to use, or incrementing to the next available id is
easier.

I'm going to skip commenting on every particular case about this.

+++ b/pylib/mozreview/mozreview/autoland/errors.py
> +AUTOLAND_TIMEOUT = WebAPIError(
> +    902,
> +    "Autoland failed to respond within the allowed time",
> +    http_status=502)  # 502 Bad Gateway
"504 Gateway Timeout".

+++ b/pylib/mozreview/mozreview/autoland/errors.py
> +BAD_AUTOLAND_CREDENTIALS = WebAPIError(
> +    903,
> +    "Bad or missing Autoland credentials.",
> +    http_status=401)  # 401 Unauthorized
I'd say this is a 500 not 401 - It's dealing with credentials but internal
server configured ones, not those passed to RB as part of the request.

Also, we're only returning this error if the credentials aren't configured, not
if they're bad credentials. I think the message should be more like that of
BAD_AUTOLAND_URL. We could probably even combine this and BAD_AUTOLAND_URL into
a single autoland configuration error.

+++ b/pylib/mozreview/mozreview/autoland/errors.py
> +BAD_UPDATE_CREDENTIALS = WebAPIError(
> +    904,
> +    "Bad or missing AutolandRequest update credentials.",
> +    http_status=401)  # 401 Unauthorized
I think we should just re-use Review Board's PERMISSION_DENIED error here rather
than rolling our own.

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

Comment 145

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3105/#review6477

Ship It!

Comment 146

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3637/#review6479

Ship It!

Comment 147

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3643/#review6481

Ship It!

Comment 148

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3739/#review6483

::: pylib/rbmozui/rbmozui/fields.py:139
(Diff revision 1)
> -            return  '<a href="%s">%s</a>' % (url, url)
> +            template = get_template('rbmozui/try_result.html')

Didn't we nuke rbmozui?

Comment 149

[Gregory Szorc [:gps]]

Comment on attachment 8562006 [details]
MozReview Request: bz://1109218/mdoglio

I left reviews on everything that was asked of me.

Comment 151

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Rename autoland_host setting to autoland_url

Comment 152

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Embed treeherder iframe to show try results (Bug 1109218)

Comment 153

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: use .last() instead of .all()[0] (bug 1109218)

Comment 154

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: rbmozui: Render changedescriptions for autoland requests.

Comment 155

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: Add API endpoint for Autoland status updates.

Comment 156

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: Test the TryAutolandTriggerResource. (Bug 1109218) r=?

Comment 157

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: rbmozui / mozreview: Initial work to expose UI and WebAPI for Try Autoland. (Bug 1109218) r=smacleod,brennie.

Comment 158

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Rename testing setting to autoland_testing

Comment 159

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Rename endpoint attribute to pingback_url

Comment 160

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Use max id instead of event_time to retrieve the last AutolandEventLogEntry

Comment 161

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Use separate fields for result and error_msg on AutolandRequestUpdateResource

Comment 162

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Update description of rev field on AutolandRequestUpdateResource

Comment 163

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)

Comment 164

[Barret Rennie [:barret] (they/them)]

Comment on attachment 8618875 [details]
MozReview Request: Use DOES_NOT_EXIST when a AutolandRequest is missing (Bug 1109218)

https://reviewboard.mozilla.org/r/3647/#review11805

Ship It!

Comment 165

[Mauro Doglio [:mdoglio]]

I opened bug 1191817 to make sure we address the issues in comment 144.