-
-
Notifications
You must be signed in to change notification settings - Fork 161
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
Conversation
getcreds
to cli for interface with get_credential
keyring/cli.py
Outdated
print(f"username: {creds.username}") | ||
print(f"password: {creds.password}") |
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.
Should we just print them newline separated without the prefix?
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 am in favor of without prefix - I only included prefix because the only other command which outputs more than one item (diagnose
) uses prefixes.
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.
What if the username or password contains a newline? Probably unlikely for both, but it's also maybe allowed?
FWIW, the diagnose
interface is meant for human consumption so can be sloppy about syntax. I'm kinda leaning the same way for the rest of the keyring CLI API - meant primarily for human consumption. Still, no reason it can't provide a reasonably stable interface that a wrapper could rely upon.
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 comment
The 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 username:
or password:
.
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 comment
The 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.
The more complex option, I guess, is to output them base64 encoded.
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. (--output json
(default 'plain'))?
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.
Added --output
argument
Help text for reference:
--output {plain,json}
Output format for 'get' operation. Default is 'plain'
Example output:
➜ keyring --get-mode creds --output json get https://example.com
{"username": "hello", "password": "world"}
➜ keyring --get-mode creds get https://example.com
hello
world
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.
Are the main users of the CLI humans? I presumed this was mostly used for programmatic interaction.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Added
--output
argumentHelp text for reference:
--output {plain,json} Output format for 'get' operation. Default is 'plain'
Example output:
➜ keyring --get-mode creds --output json get https://example.com {"username": "hello", "password": "world"} ➜ keyring --get-mode creds get https://example.com hello world
I'm liking this a lot! Thanks. Is it possible to use --mode
(since get
is implied by the command)?
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.
Done
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'm late to the party)
Are the main users of the CLI humans? I presumed this was mostly used for programmatic interaction.
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.
If another datapoint is useful: pip has support for invoking keyring
both via python import and as a subprocess, and the author "recommend[s] you use the subprocess
provider.
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
keyring/cli.py
Outdated
@@ -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"] |
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.
self.parser._operations = ["get", "set", "del", "getcreds", "diagnose"] | |
self.parser._operations = ["get", "set", "del", "get-creds", "diagnose"] |
But then I notice we have get
and get-creds
and the asymetry is bothering me, and it makes me wonder if it should be keyring get password
+ keyring get credential
(or get pass
and get creds
) or maybe get
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 think get
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.
I think get with some sort of --mode might make the most sense.
This would be my personal vote over separate command. Updated PR to add --get-mode
with default password
Help text for reference:
--get-mode {password,creds}
Mode for 'get' operation. 'password' requires a username and will return only the password.
'creds' does not require a username and will return both the username and password separated by
a newline. Default is 'password'
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.
I don't really understand how we're going to use this downstream though.
If no username on the index url, use --get-mode creds
else use default. After that, the cacheing by netloc should just work
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.
Yeah but we won't know if the user's keyring version supports this without trial and error.
You can tell this is the problem on the first call if the status code is 2
instead of 1
and don't try again after first 2
status.
➜ keyring get --get-mode creds https://example.com/ oauth2accesstoken
usage: keyring [-h] [-p KEYRING_PATH] [-b KEYRING_BACKEND] [--list-backends]
[--disable] [--print-completion {bash,zsh,tcsh}]
[{get,set,del,diagnose}] [service] [username]
keyring: error: unrecognized arguments: --get-mode creds https://example.com/
X➜ echo $? +
2
➜ keyring get https://example.com/ oauth2accesstoken
X➜ echo $? +
1
The user manually requests keyring
use, so uv
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.
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.
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 was 2
.
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
getcreds
to cli for interface with get_credential
get
options to cli for interface with get_credential
and support JSON output
…ic and reduces cyclomatic complexity to 2.
Releasing as v25.2.0. |
Internally,
keyring
supportsget_credential
, which is useful for backends which can supply their own username. This adds an exposedget_credential
in the cli -keyring --mode creds get [service] [OPTIONAL - username]
This also adds support for JSON output type via
--output json
optionHelp text for reference:
Example output: