search

dapr / python-sdk

Dapr SDK for Python
Apache License 2.0
230 stars 128 forks source link

Error handling #659

Closed elena-kolevska closed 8 months ago

elena-kolevska commented 9 months ago

Description

Improves developer experience on error handling for the new Rich gRPC Errors from Dapr

Issue reference

https://github.com/dapr/python-sdk/issues/648

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

elena-kolevska commented 9 months ago

@berndverst pls let me know if the DaprGrpcError design and naming look ok to you. I would also like to get your opinion on the integration testing approach I took. I think it can be helpful for other cases too.

codecov[bot] commented 9 months ago

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (ef73209) 86.21% compared to head (682f645) 86.37%.

Files Patch % Lines
dapr/clients/exceptions.py 92.75% 5 Missing :warning:
dapr/clients/grpc/client.py 95.23% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #659 +/- ## ========================================== + Coverage 86.21% 86.37% +0.15% ========================================== Files 79 79 Lines 3998 4094 +96 ========================================== + Hits 3447 3536 +89 - Misses 551 558 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

berndverst commented 9 months ago

LGTM. Can you resolve the conflict now that I merged the ruff stuff :)

elena-kolevska commented 9 months ago

Marking this ready so the code can be reviewed while I'm working on examples and docs.

elena-kolevska commented 9 months ago

Ok, examples and docs are ready :)

elena-kolevska commented 9 months ago

I addressed the review comments and I simplified the DaprGrpcError structure. 🙏

berndverst commented 9 months ago

I now updated the tox configuration to include your error handling test app. I am expected it to fail (it fails for me locally). You'll have to debug that one some more.

berndverst commented 9 months ago

If this PR requires Dapr 1.13, please let me know. In that case we will not be able to successfully run the CI tests until we have Dapr 1.13.0rc1 tagged

elena-kolevska commented 9 months ago

If this PR requires Dapr 1.13, please let me know. In that case we will not be able to successfully run the CI tests until we have Dapr 1.13.0rc1 tagged

Yes, that's exactly it! I could only test it with my local dapr build and had to make the following modifications to the README to make it pass:

Unfortunately, I had also commented out all the other examples in the tox.ini so it only ran my test while I was working on it, to keep it faster, but when I was committing, I also reverted the line that added my errors test by mistake. Sorry about that! That explains why the CI test succeeded when I was expecting it to fail. I thought you were doing some voodoo by making dapr cli build from a custom commit. If that is possible, btw, please let me know how it's done :)

berndverst commented 9 months ago

I will rerun tests and merge this when 1.13.0rc1 has been tagged. At that point this should all pass :) (I tried it locally with a Dapr build from master)