Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output error message for provider failures #578

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

pyoor
Copy link
Contributor

@pyoor pyoor commented Oct 24, 2019

Currently, provider error messages are obscured by django request exceptions. This PR adds a ProviderExceptions class and returns error messages containing the provider response.

@pyoor pyoor requested a review from choller October 24, 2019 17:30
@codecov-io
Copy link

codecov-io commented Nov 13, 2019

Codecov Report

Merging #578 into master will decrease coverage by 0.07%.
The diff coverage is 27.27%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #578      +/-   ##
=========================================
- Coverage   67.17%   67.1%   -0.08%     
=========================================
  Files         111     111              
  Lines        8195    8204       +9     
=========================================
  Hits         5505    5505              
- Misses       2690    2699       +9
Impacted Files Coverage Δ
server/crashmanager/Bugtracker/BugzillaREST.py 10.71% <0%> (+0.09%) ⬆️
server/crashmanager/Bugtracker/Provider.py 67.74% <100%> (+2.22%) ⬆️
server/crashmanager/views.py 65.11% <20%> (-0.2%) ⬇️
server/crashmanager/Bugtracker/BugzillaProvider.py 21.59% <21.42%> (+0.43%) ⬆️
FTB/Signatures/CrashInfo.py 90.17% <0%> (-0.43%) ⬇️
Collector/Collector.py 65.32% <0%> (-0.31%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 315bb61...499c4bb. Read the comment docs.

raise RuntimeError("Failed to create bug: %s", ret)
if ret.status_code != 200:
content = json.loads(ret.content)
raise BugzillaProviderException(content['message'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably try to handle more error cases here:

  1. The resulting JSON might miss the message.
  2. ret.content might not even parse as JSON (if the server has a hickup and returns nothing or e.g. the hardhat page)

In both cases, this would cause a 500 again. Instead we should probably wrap these cases in a BugzillaProviderException as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants