-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
feature: Add get
options to cli for interface with get_credential
and support JSON output
#678
Changes from 4 commits
0a104c8
95e4bb4
9773fc4
8ee16b6
9a2d951
7830a64
bd1a7ca
55432e9
1c6450a
98ce17a
03801d7
734a17d
f922be6
33bae95
49e7cf8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
get_password, | ||
set_keyring, | ||
set_password, | ||
get_credential, | ||
) | ||
from .util import platform_ | ||
|
||
|
@@ -41,7 +42,7 @@ def __init__(self): | |
self.parser.add_argument( | ||
"--disable", action="store_true", help="Disable keyring and exit" | ||
) | ||
self.parser._operations = ["get", "set", "del", "diagnose"] | ||
self.parser._operations = ["get", "set", "del", "getcreds", "diagnose"] | ||
self.parser.add_argument( | ||
'operation', | ||
choices=self.parser._operations, | ||
|
@@ -81,7 +82,10 @@ def run(self, argv): | |
|
||
def _check_args(self): | ||
if self.operation: | ||
if self.service is None or self.username is None: | ||
if self.operation == 'getcreds': | ||
if self.service is None : | ||
self.parser.error(f"{self.operation} requires service") | ||
elif self.service is None or self.username is None: | ||
self.parser.error(f"{self.operation} requires service and username") | ||
|
||
def do_get(self): | ||
|
@@ -90,6 +94,13 @@ def do_get(self): | |
raise SystemExit(1) | ||
print(password) | ||
|
||
def do_getcreds(self): | ||
creds = get_credential(self.service, self.username) | ||
if creds is None: | ||
raise SystemExit(1) | ||
print(f"username: {creds.username}") | ||
print(f"password: {creds.password}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just print them newline separated without the prefix? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am in favor of without prefix - I only included prefix because the only other command which outputs more than one item ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the username or password contains a newline? Probably unlikely for both, but it's also maybe allowed? FWIW, the I'm okay without the prefix as long as the help guides the user as to what they're getting from the output. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it allowed for the username or password to include a newline? I guess the keyring is arbitrary enough that it certainly could, but then it could also include The more complex option, I guess, is to output them base64 encoded. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at the tests and they do test for difficult characters on every keyring, so in theory they are supported. Still, I just wanted to flag it as a possibility.
Oof. That wouldn't be user friendly. I'm thinking JSON would be better for safety, but that's not very friendly either. Maybe there should be an option to emit in an unambiguous form like JSON. ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added Help text for reference:
Example output:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm unsure. In my personal usage, I always use the CLI as a human and always use the library for programming, but that's because I'm always coding in Python. It's my impression that my experience has been the most common experience for a long time but that may be changing, especially with the introduction of keyring to homebrew and the use of it by tools like uv. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm liking this a lot! Thanks. Is it possible to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (sorry I'm late to the party)
If another datapoint is useful: pip has support for invoking edit: BTW, thanks @BakerNet for this feature! It allows us to bring pip's keyring subprocess provider up to feature parity with pip's keyring import provider: pypa/pip#12748 |
||
|
||
def do_set(self): | ||
password = self.input_password( | ||
f"Password for '{self.username}' in '{self.service}': " | ||
|
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.
When naming things, I tend to separate words with something (space, dash, underscore). Here, I think dash may be appropriate.
But then I notice we have
get
andget-creds
and the asymetry is bothering me, and it makes me wonder if it should bekeyring get password
+keyring get credential
(orget pass
andget creds
) or maybeget
with--creds
or--password
(default). All of those options have more onerous transitional concerns, so I'm not loving any option.I don't feel strongly about it, so happy to move forward with something pragmatic.
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.
get-creds
seems like simplest transition for the API, I feel your pain though. Perhaps it'd be worth opening a tracking issue to consider a different API in some future major version. I thinkget
with some sort of--mode
might make the most sense. You could also just assume a single value is the service name and that the username is null and return the full credentials in that case? eek.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.
This would be my personal vote over separate command. Updated PR to add
--get-mode
with defaultpassword
Help text for reference:
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.
I don't really understand how we're going to use this downstream though. We'll need to try try
--get-mode creds
first and if it fails fall back to excluding the flag? Presumably there will be a different exit code? I guess we'll cache this so we don't need to do it on every request? but we have a lot of concurrent requests that make this difficult to do efficiently.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.
If no username on the index url, use
--get-mode creds
else use default. After that, the cacheing by netloc should just workThere 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.
You can tell this is the problem on the first call if the status code is
2
instead of1
and don't try again after first2
status.The user manually requests
keyring
use, souv
would already be failing for them if no username provided. This is just a different failure mode.if status code 2, warn user to either use username in index-url or update to keyring version X
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.
Err... I think I see an issue - for index-urls which don't need keyring (PyPi) - you'd hit the status code
2
when keyring requested but don't know whether you need to warn the user about it.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.
On this point, you're using a mutex on the keyring cache, so the first time a host fails the check, you're returning None for all other checks on that host (keyring only running once per host). The only way you'll run into new efficiency issues is if there are a large number of unique index-url hosts involved.
This seems unlikely - but if you want to avoid it, you could check the exit code of the command and skip subsequent
keyring
calls if status code was2
.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.
👍 Sorry I didn't mean to distract, unless usage in
uv
helps inform the design here I don't think we need to discuss it too much.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.
@zanieb We're kind of off-topic to discussion of this PR - can move to draft example PR on
uv
here:astral-sh/uv#3081