Raise an error when a HostingServiceAccount fails to load a hosting service.
Review Request #12667 — Created Oct. 4, 2022 and submitted
Information | |
---|---|
maubin | |
Review Board | |
release-5.0.x | |
Reviewers | |
reviewboard | |
Repositories use their associated
HostingServiceAccount
to fetch hosting
services. The hosting service account will returnNone
if a hosting service
fails to load. This isn't the greatest thing to do since this hides the fact
that the service didn't load, and instead makes the repository behave as if
they do not have a hosting service configured.This change instead raises an error when a
HostingServiceAccount
fails to
load a service. This way, a more appropriate and helpful eror message is shown
when, for example, trying to post changes or view diffs linked to repositories
that use a hosting service that can't be loaded.This also adds some help texts to the
HostingServiceAccount
's fields. This
was primarily done to clarify that theservice_name
field should be set to
the hosting service ID rather than the hosting service name. This assumption
is made everywhere in the code except for in one test file which sets the
field to a hosting service name, which we correct in this change.
- Ran all unit tests.
- Tested viewing review requests and diffs that are from a repository that uses
a hosting service that can't be loaded, saw theHostingServiceError
page
instead. - Tested
rbt post REVISION
with a git revision, saw a
500: Internal Server Error
response and saw theHostingServiceError
get
raised in the server logs.
Summary |
---|
Description | From | Last Updated |
---|---|---|
With the API, is a 500: Internal Server Error an appropriate response for whenever the HostingServiceError is raised? Should the … |
![]() |
|
I'm not sure how to describe these fields, i.e. not sure what I should put in the help text. |
![]() |
|
Blank line between class docstrings and members. |
|
|
I expected this would be an instance of the hosting service, but of course it makes sense it's not. Just … |
|
|
The last parameter of these new-fangled function definitions using typing should always have a trailing comma. |
|
|
Rather than have two places where we invoke the constructor, it's often better to build the message up-front and then … |
|
|
For instance variables, the new pattern we're starting to use for typing is to put the following in the class … |
|

Testing Done: |
|
---|

-
-
With the API, is a
500: Internal Server Error
an appropriate response for whenever theHostingServiceError
is raised? Should the response be something that is more specific to the error? -
reviewboard/hostingsvcs/models.py (Diff revision 1) I'm not sure how to describe these fields, i.e. not sure what I should put in the help text.

Change Summary:
Updated docstrings of methods that might raise a
HostingServiceError
.
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+206 -22) |
Checks run (2 succeeded)

Change Summary:
- Created a
MissingHostingServiceError
with a better error message (and can include repository information). - Added more unit tests.
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+332 -24) |
Checks run (2 succeeded)

Change Summary:
Added help texts for
HostingServiceAccount
'sdata
andvisible
fields.
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+346 -28) |
Checks run (2 succeeded)
-
-
reviewboard/hostingsvcs/errors.py (Diff revision 4) Blank line between class docstrings and members.
-
reviewboard/hostingsvcs/errors.py (Diff revision 4) I expected this would be an instance of the hosting service, but of course it makes sense it's not. Just from the parameter name. How about we call this
hosting_service_id
and make sure the ID is passed in instead of the name? The ID will better help identify the missing service. -
reviewboard/hostingsvcs/errors.py (Diff revision 4) The last parameter of these new-fangled function definitions using typing should always have a trailing comma.
-
reviewboard/hostingsvcs/errors.py (Diff revision 4) Rather than have two places where we invoke the constructor, it's often better to build the message up-front and then pass it to the constructor.
This pattern helps as classes grow in complexity. If, say, we wanted to allow the caller to override the message or add additional arguments that went to
HostingServiceError
, then it'd help to conditionalize the message but not have to update the parent constructor call in more than one place. -
reviewboard/hostingsvcs/errors.py (Diff revision 4) For instance variables, the new pattern we're starting to use for typing is to put the following in the class body before functions:
###################### # Instance variables # ###################### hosting_service: str

Change Summary:
- Some formatting and doc fixes.
- Changed
hosting_service
parameter tohosting_service_id
to be more explicit. - Avoids calling the parent constructor for
MissingHostingServiceError
in more than one place.
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+384 -28) |