-
Notifications
You must be signed in to change notification settings - Fork 7
Improve error reporting for operator-courier related errors #49
Improve error reporting for operator-courier related errors #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I had few comments
docs/usage/v1.md
Outdated
@@ -44,14 +44,19 @@ Error messages have following format: | |||
"message": "<detailed error description>", | |||
} | |||
``` | |||
There may potentially be other fields providing further details, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be more explicit which particular errors will provide that information. It can be part of explanation cell IMO
omps/errors.py
Outdated
@@ -13,6 +13,14 @@ class OMPSError(Exception): | |||
"""Base OMPSError""" | |||
code = 500 | |||
|
|||
def to_json(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a JSON, it returns python dict, this method should be called differently: to_dict
for example.
omps/errors.py
Outdated
def to_json(self): | ||
data = super().to_json() | ||
if self.validation_info is not None: | ||
data.update({'validation_info': self.validation_info}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should always return that key in response, but with empty dict
omps/errors.py
Outdated
def to_json(self): | ||
data = super().to_json() | ||
if self.quay_response is not None: | ||
data.update({'quay_response': self.quay_response}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should always return that key in response, but with empty dict when undefined
Please set requirement for operator-courier >= 1.2.1 in setup.py |
8e68fa2
to
f69dafd
Compare
Made the changes as @MartinBasti requested. |
omps/errors.py
Outdated
@@ -13,6 +13,14 @@ class OMPSError(Exception): | |||
"""Base OMPSError""" | |||
code = 500 | |||
|
|||
def to_dict(self): | |||
"""Turn the exception into json data for use as an error response""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/json/something else/ :-)
omps/errors.py
Outdated
|
||
def to_dict(self): | ||
data = super().to_dict() | ||
data.update({'quay_response': self.quay_response}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not data['quay_response'] = self.quay_response
when we update only one item
… issues Signed-off-by: Adam Cmiel <acmiel@redhat.com>
Signed-off-by: Adam Cmiel <acmiel@redhat.com>
Signed-off-by: Adam Cmiel <acmiel@redhat.com>
Signed-off-by: Adam Cmiel <acmiel@redhat.com>
Signed-off-by: Adam Cmiel <acmiel@redhat.com>
f69dafd
to
f289691
Compare
Rebased and fixed issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Addresses #29.