-
Notifications
You must be signed in to change notification settings - Fork 473
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 create_repo
with exists_ok
but no permission
#1364
Conversation
The documentation is not available anymore as the PR was closed or merged. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1364 +/- ##
==========================================
- Coverage 84.50% 84.50% -0.01%
==========================================
Files 48 48
Lines 4814 4820 +6
==========================================
+ Hits 4068 4073 +5
- Misses 746 747 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptual LGTM, but let's wait for @LysandreJik's review :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-conceptual LGTM :)
Corner case but used in a lot of integrations. Problem is when doing
create_repo(..., exists_ok)
. If repo already exists, it's not supposed to raise an exception. However, if the repo exists but the user do not have permission on this namespace, a HTTP 403 is thrown. Since we catch only HTTP 409 conflict, the error is not caught.With this PR, if we get an HTTP 403, we check if the repo exists or not before raising an exception. This is useful for third-party integrations having a "push_to_hub"-like method and
create_pr=True
. We do not want to throw an error on create repo since write permission will not be needed to create a PR.See related slack thread (internal link) cc @thomasw21 @coyotte508