Closed Bug 1191808 Opened 6 years ago Closed 6 years ago

Change all the custom WebAPIError codes to be greater than 1000

Categories

(MozReview Graveyard :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mdoglio, Assigned: mdoglio)

References

Details

Attachments

(3 files)

From https://bugzilla.mozilla.org/show_bug.cgi?id=1109218#c144:

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.
Blocks: 1191817
Priority: -- → P2
Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod
Attachment #8651802 - Flags: review?(smacleod)
Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod
Attachment #8651803 - Flags: review?(smacleod)
Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod
Attachment #8651806 - Flags: review?(smacleod)
Comment on attachment 8651802 [details]
MozReview Request: Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod

https://reviewboard.mozilla.org/r/16987/#review15235

::: pylib/mozreview/mozreview/errors.py:13
(Diff revision 1)
>  

Can you update the commit message to include the reason for this change please.
Attachment #8651802 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/16987/#review15237

::: pylib/mozreview/mozreview/autoland/resources.py:32
(Diff revision 1)
> -from mozreview.decorators import webapi_scm_groups_required
> +                              NOT_PUSHED_PARENT_REVIEW_REQUEST,

The trailing paren `)` should be added in this commit not the next, so this commit isn't broken.
Comment on attachment 8651803 [details]
MozReview Request: Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

https://reviewboard.mozilla.org/r/16989/#review15239

::: pylib/mozreview/mozreview/autoland/resources.py:31
(Diff revision 1)
> -                              NOT_PUSHED_PARENT_REVIEW_REQUEST,
> +                              NOT_PUSHED_PARENT_REVIEW_REQUEST)

Make sure to move this change to the first commit.

::: pylib/mozreview/mozreview/errors.py:52
(Diff revision 1)
>  BAD_UPDATE_CREDENTIALS = WebAPIError(
>      1008,
>      "Bad or missing AutolandRequest update credentials.",
>      http_status=401)  # 401 Unauthorized

I don't believe 1008 is checked or used by anyone yet (and 1007 wasn't either). How about we just slip in a change to make this 1007 so there aren't any gaps :)
Attachment #8651803 - Flags: review?(smacleod)
Comment on attachment 8651803 [details]
MozReview Request: Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

https://reviewboard.mozilla.org/r/16989/#review15241

Meant to put a ship-it on the last review.
Attachment #8651803 - Flags: review+
Comment on attachment 8651806 [details]
MozReview Request: Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod

https://reviewboard.mozilla.org/r/16991/#review15243

::: pylib/mozreview/mozreview/autoland/resources.py:302
(Diff revision 1)
> -            return BAD_UPDATE_CREDENTIALS
> +            return PERMISSION_DENIED

Even though we're using the built in codes from review board for these things, you can still provide a custom message if you think it's useful.

See the method on `WebAPIError` and the example in its docstring here: https://github.com/djblets/djblets/blob/6c8dd31b0bd49e0d0ecad4cce1718b923fc4e7dd/djblets/webapi/errors.py#L52-L58
Attachment #8651806 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/16989/#review15239

> I don't believe 1008 is checked or used by anyone yet (and 1007 wasn't either). How about we just slip in a change to make this 1007 so there aren't any gaps :)

Ah, I see in the next commit you remove this. NVM, carry on.
Duplicate of this bug: 1191811
Comment on attachment 8651802 [details]
MozReview Request: Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod

Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod

Error codes should be greater than 1000 (which RB core guarantees it will not
use itself). Let's  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.
Attachment #8651802 - Flags: review+ → review?(smacleod)
Comment on attachment 8651803 [details]
MozReview Request: Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod
Attachment #8651803 - Flags: review+ → review?(smacleod)
Comment on attachment 8651806 [details]
MozReview Request: Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod

Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod
Attachment #8651806 - Flags: review+ → review?(smacleod)
Assignee: nobody → mdoglio
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8651802 - Flags: review?(smacleod) → review+
Attachment #8651803 - Flags: review?(smacleod) → review+
Attachment #8651806 - Flags: review?(smacleod) → review+
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.