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: remove exception when keyring is locked #6612

Closed
wants to merge 1 commit into from

Conversation

MatthieuBizien
Copy link
Contributor

Pull Request Check List

Resolves: #1917

  • Added tests for changed code.
  • Updated documentation for changed code.

@neersighted
Copy link
Member

Thanks for starting on this! There are many more errors Keyring has been throwing (one of the reasons no one has started on this yet) -- see all the dupes of #1917 for situations we should handle.

We also most definitely need a unit/regression test for this.

@neersighted neersighted added area/error-handling Bad error messages/insufficient error handling kind/enhancement Not a bug or feature, but improves usability or performance area/auth Related to the authenticator and keyring labels Sep 23, 2022
@neersighted neersighted marked this pull request as draft September 24, 2022 13:16
@tasansal
Copy link

tasansal commented Nov 2, 2022

When will this be merged? We get this a lot on our enterprise HPC at work and have to run a workaround environment variable all the time.

@real-yfprojects
Copy link
Contributor

Thanks for starting on this! There are many more errors Keyring has been throwing (one of the reasons no one has started on this yet) -- see all the dupes of #1917 for situations we should handle.

I was looking through all linked issues for Exceptions poetry could catch to prevent this. I bumped into some problems. Sometimes the keyring raises a RuntimeError. I guess discarding that isn't a problem. However how to catch win32ctypes.pywin32.pywintypes.error which is a direct subclass of Exception?
There even was an incident where keyring raised a ModuleNotFoundError. IMO the last two are issues of keyring and should be handled by that library. However we could also simply catch all Exceptions.

@dimbleby
Copy link
Contributor

I would agree with you: catching keyring.errors.KeyringError seems like the right thing to do, and if keyring raises other errors then that feels to me like a bug that should be raised against that project.

That ought to catch the great majority of cases; and if keyring does take the view that it can raise any old exception and callers are supposed to handle them all - well that will be a simple enough thing to change later.

@real-yfprojects
Copy link
Contributor

Ok, so I opened an updated PR: #8227

Copy link

github-actions bot commented Mar 3, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/auth Related to the authenticator and keyring area/error-handling Bad error messages/insufficient error handling kind/enhancement Not a bug or feature, but improves usability or performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyring errors during non-publishing operations
5 participants