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

Catch 403 Error for Artifactory backend paths #27

Merged
merged 3 commits into from
Sep 28, 2021
Merged

Conversation

hagenw
Copy link
Member

@hagenw hagenw commented Sep 28, 2021

Closes #26

When accessing a non-existing path or a path where you don't have the permission to access it, you might experience a 403 error returned from Artifactory. This started happening only since a few days and most likely it depends on the version of dohq-artifactory and maybe if you have anonymous access enabled or not.

Here, we don't care, but just catch the corresponding error in order to return a proper False for exists() and an empty list for glob().

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #27 (24cf0e9) into master (998e871) will not change coverage.
The diff coverage is 100.0%.

Impacted Files Coverage Δ
audbackend/core/artifactory.py 100.0% <100.0%> (ø)

@hagenw
Copy link
Member Author

hagenw commented Sep 28, 2021

I was not really able to create a test to catch the new 403 error as this would require setting up another repository and most likely disable anonymous access, etc.
So for the short term I would propose we just include the fix, but stay with the non-coverage testing.

@hagenw hagenw merged commit 1c66829 into master Sep 28, 2021
@hagenw hagenw deleted the cover-403-error branch September 28, 2021 12:06
or
``requests.exceptions.HTTPError: 403 Client Error``
error,
which might depend on the instaleld ``dohq-artifactory``
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
which might depend on the instaleld ``dohq-artifactory``
which might depend on the installed ``dohq-artifactory``

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, too late.

As we need this fix as fast as possible I decided to go without review.

return audfactory.path(path).exists()
except RuntimeError: # pragma: nocover
except self._non_existing_path_error: # pragma: nocover
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it make sense to catch any Exception here to avoid the code breaks again in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

In general it is not a good idea to catch simply everything, because you might also get an exception because you use your keyboard to interrupt or you don't have an internet connection. Those cases we would then also catch.

On the other hand it will make sense easier for us to maintain.

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.

Artifactory backend migth fail with 403 error
2 participants