-
Notifications
You must be signed in to change notification settings - Fork 131
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
[ISSUE] Regression in PermissionDefined
error handling
#755
Comments
This is a big issue causing issues in UCX that fail assessment runs: |
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this issue
Sep 13, 2024
## Changes #741 introduced a change to how an error message was modified in `ApiClient._perform`. Previously, arguments to the DatabricksError constructor were modified as a dictionary in `_perform`. After that change, `get_api_error` started to return a `DatabricksError` instance whose attributes were modified. The `message` attribute referred to in that change does not exist in the DatabricksError class: there is a `message` constructor parameter, but it is not set as an attribute. This PR refactors the error handling logic slightly to restore the original behavior. In doing this, we decouple all error-parsing and customizing logic out of ApiClient. This also sets us up to allow for further extension of error parsing and customization in the future, a feature that I have seen present in other SDKs. Fixes #755. ## Tests <!-- How is this tested? Please see the checklist below and also describe any other relevant tests --> - [ ] `make test` run locally - [ ] `make fmt` applied - [ ] relevant integration tests applied
aravind-segu
pushed a commit
to aravind-segu/databricks-sdk-py
that referenced
this issue
Sep 18, 2024
## Changes databricks#741 introduced a change to how an error message was modified in `ApiClient._perform`. Previously, arguments to the DatabricksError constructor were modified as a dictionary in `_perform`. After that change, `get_api_error` started to return a `DatabricksError` instance whose attributes were modified. The `message` attribute referred to in that change does not exist in the DatabricksError class: there is a `message` constructor parameter, but it is not set as an attribute. This PR refactors the error handling logic slightly to restore the original behavior. In doing this, we decouple all error-parsing and customizing logic out of ApiClient. This also sets us up to allow for further extension of error parsing and customization in the future, a feature that I have seen present in other SDKs. Fixes databricks#755. ## Tests <!-- How is this tested? Please see the checklist below and also describe any other relevant tests --> - [ ] `make test` run locally - [ ] `make fmt` applied - [ ] relevant integration tests applied
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
The 0.32.0 release included some changes (#741) that refactored error handling. There's a regression in the handling for
PermissionDenied
that results in anAttributeError
being raised because the code tries to setmessage
onPermissionDenied
, but that attribute isn't present.Reproduction
On an account where you don't have permission to create a group, raising
PermissionDenied
fails:Expected behavior
The
PermissionDenied
error should be raised correctly:Is it a regression?
This is a regression introduced in 0.32.0 against 0.31.1. (It is not fixed in 0.32.1.)
Debug Logs
Logs are included in the examples above.
Other Information
Additional context
We (UCX) discovered this when users reported difficulty understanding the cause of a (permissions-related) error: databrickslabs/ucx#2542
The text was updated successfully, but these errors were encountered: