-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 #8227
fix: remove exception when keyring is locked #8227
Conversation
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.
review of #6612 indicated that unit test would be required, which seems reasonable.
I'd suggest adding a LockedBackend
to tests/conftest.py
- cf the existing DummyBackend
etc - and writing a test that uses that in test_authenticator.py
.
Ok so I added a few tests but I am very unsure whether I did it correctly and sufficiently. Please review. |
f009d7c
to
d507f5a
Compare
tests look plausible to me. I'm not a fan of the many imports of |
Me neither, but I thought there must a reason the existing code only imports keyring on demand instead of adding the imports to the top of the file. |
There''s already a keyring import at the top of the file, I see no reason to be afraid of another Line 17 in 2b16b2f
|
d507f5a
to
9f868fc
Compare
Ok, fixed |
You only tested one of your |
56ad263
to
ca38ef4
Compare
* src/poetry/utils/password_manager.py (PoetryKeyring.get_credential)
* tests/conftest.py : Implement `LockedBackend` inheriting `KeyringBackend` and throwing `KeyringLocked` on access. * tests\utils\test_authenticator.py : Test that locked keyrings still lead to a request. * tests\utils\test_password_manager.py : Test that locked keyrings do not fail.
* tests/utils/test_authenticator.py * tests/conftest.py
7175b88
to
aea0957
Compare
Had to manually set backend keyring for private and public reposFantastic to see keyring improvements being released in this version. However I still had a convoluted route to getting authentication working when trying to install packages from google artifact registry over SSH (5 hours in total). I know 1.7.0 is not released yet so there are still some bugs expected but since this issue is closed I thought I would log here all the loops I had to jump through in case it is helpful in finding any more bugs that might have been missed. The crux of the issue was I had to set the backend manually depending on if I was installing from public or private repos. Install process
curl -sSL https://install.python-poetry.org | python3 -
curl -sSL https://install.python-poetry.org | python3 - --git https://github.com/python-poetry/poetry.git@master
poetry self add keyrings.google-artifactregistry-auth
This then completed successfully for me. I hope it is helpful. Please let me know if any more info is required or if I am making a silly mistake. Thanks! System: |
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. |
Resolves: #1917
This inherits and therefore closes #6612.
KeyringError
andRuntimeError
when accessing keyring.