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

fix(az): catch HttpResponseError in _check_hns #487

Merged
merged 2 commits into from
Nov 30, 2024

Conversation

M0dEx
Copy link

@M0dEx M0dEx commented Nov 21, 2024

Fixed an uncaught exception in check_hns, which can fail with the HttpResponseError when supplied with a DefaultAzureCredential.

Closes #486.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.2%. Comparing base (7f1a5dc) to head (01e4eea).

Files with missing lines Patch % Lines
cloudpathlib/azure/azblobclient.py 50.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #487     +/-   ##
========================================
- Coverage    94.4%   93.2%   -1.3%     
========================================
  Files          23      23             
  Lines        1776    1776             
========================================
- Hits         1678    1656     -22     
- Misses         98     120     +22     
Files with missing lines Coverage Δ
cloudpathlib/azure/azblobclient.py 87.8% <50.0%> (-9.2%) ⬇️

... and 2 files with indirect coverage changes

---- 🚨 Try these New Features:

@@ -202,7 +202,7 @@ def _check_hns(self, cloud_path: AzureBlobPath) -> Optional[bool]:
try:
account_info = self.service_client.get_account_information() # type: ignore
self._hns_enabled = account_info.get("is_hns_enabled", False) # type: ignore
except ResourceNotFoundError:
except (ResourceNotFoundError, HttpResponseError):
Copy link
Member

Choose a reason for hiding this comment

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

Per the stack trace in #486, can we confirm the error code is ErrorCode:AuthorizationPermissionMismatch and if not, re-raise the HttpResponseError? That will make other potential errors easier to track down.

Copy link
Author

Choose a reason for hiding this comment

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

Is checking for HTTPStatus.FORBIDDEN sufficient?

@M0dEx M0dEx force-pushed the azure-permission-mismatch branch from 01e4eea to a957e16 Compare November 24, 2024 11:13
@pjbull pjbull changed the base branch from master to 487-live-tests November 30, 2024 00:14
@pjbull pjbull merged commit 8c9a013 into drivendataorg:487-live-tests Nov 30, 2024
5 of 6 checks passed
@pjbull pjbull mentioned this pull request Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure: _check_hns throws an HttpResponseError
2 participants