Fall back when keyring is locked #7037
Conversation
|
see #6612 |
There was a problem hiding this comment.
Needs tests (and mostly looks like a less correct #6612 -- @MatthieuBizien, are you still working on that PR)?
We also do not accept PRs against the 1.2 branch -- this change must go into master to be accepted.
| @@ -46,9 +46,14 @@ def get_credential( | |||
| return default | |||
|
|
|||
| import keyring | |||
| import keyring.errors | |||
| credential = keyring.get_credential(name, username) | ||
| except keyring.errors.KeyringLocked: | ||
| self._is_available = False | ||
| break |
There was a problem hiding this comment.
This flow-control doesn't make sense (we shouldn't abort trying all names) -- instead, defaulting credential to None and logging make more sense.
There was a problem hiding this comment.
If the keyring is locked because the user canceled, asking them again and again is not very good UX. A single poetry install command might prompt the user a hundred times...
remram44
force-pushed
the
keyring-locked
branch
from
35e8918
to
e55f646
Compare
|
The only option to avoid this is to make the fallback behavior of |
| credential = keyring.get_credential(name, username) | ||
| try: | ||
| credential = keyring.get_credential(name, username) | ||
| except keyring.errors.KeyringLocked: |
There was a problem hiding this comment.
This is missing the #3012 error which is common for headless servers.
There was a problem hiding this comment.
I feel like no retry should happen, even with different service names, when getting a KeyringLocked, but I am not sure that behavior is appropriate for all the other KeyringError subclasses.
|
Closing because #8227 has been merged. |
|
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. |
Pull Request Check List
Resolves: #1917
Description
Fall back if keyring is locked.
This avoids Poetry crashing with KeyringLocked if no keyring is available or the user doesn't unlock it, in situation where no credentials are needed.
If credentials were needed and the keyring is locked, the user will get a permission error, which I believe is perfectly fine.
I think Poetry shouldn't request access to the keyring before the remote server has actually asked for credentials, but this can be done in a subsequent change. #1917 is affecting many users.