Support username and password from keyring #1734
Conversation
funkyfuture
left a comment
funkyfuture
left a comment
There was a problem hiding this comment.
sorry that i missed this aspect back then. from looking at Github's web ui this lgtm. @ofek, would it be of help for you if i had a closer look with an IDE on this?
|
Yes please |
tests/utils/test_auth.py
Outdated
| class MockKeyring: | ||
| @staticmethod | ||
| def get_credential(*_): | ||
| class Credential: | ||
| username = 'gat' | ||
|
|
||
| return Credential() | ||
|
|
||
| @staticmethod | ||
| def get_password(*_): | ||
| return 'guido' |
There was a problem hiding this comment.
i have no strong opinion on this, but it would imo make sense to
a) realize this as a test fixture that is automatically used on all tests in this module.
b) add the password attribute to the Credential dummy.
There was a problem hiding this comment.
Thanks for the review. Good idea :)
| self.__username_was_read = True | ||
| return self._app.prompt(f"Username for '{self._repo_config['url']}' [__token__]") or '__token__' | ||
|
|
||
| def _read_keyring(self) -> str | None: |
There was a problem hiding this comment.
this could also store the __password property as i'm assuming that using a username from this credentials source doesn't make sense with a password from another source.
There was a problem hiding this comment.
You are right, no need to run read the password again if we already have it. I pushed my changes. Thanks for the review
stollero
force-pushed
the
fix-keyring-auth
branch
from
a0e624f to
1056b7b
Compare
funkyfuture
left a comment
funkyfuture
left a comment
There was a problem hiding this comment.
thanks, beside the minor import detail this looks good to merge.
(don't know whether the UI changed or it's circumstances. i can't mark the inline comment threads as resolved. they are.)
src/hatch/publish/auth.py
Outdated
| @@ -1,5 +1,7 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| import keyring | |||
There was a problem hiding this comment.
i'm pretty sure @ofek prefers that to be imported only when it's needed in _read_keyring.
|
@ofek not sure whether i wasn't loud enough. this is a fine fix for a regression i introduced and i would be thankful if it could get merged. |
stollero
force-pushed
the
fix-keyring-auth
branch
from
aa81356 to
56489c1
Compare
stollero
force-pushed
the
fix-keyring-auth
branch
from
56489c1 to
ad3c045
Compare
stollero
force-pushed
the
fix-keyring-auth
branch
from
ad3c045 to
6310cd4
Compare
3 participants
Hello everyone,
this brings back the enhancement added with #713 that broke with f59508b.
Had minor problems getting the tests to run because https://hatch.pypa.io/latest/community/contributing/ is outdated, but https://github.com/pypa/hatch/blob/master/docs/community/contributing.md worked 👍.