Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add 'keyring list' command to CLI #3331
feat: Add 'keyring list' command to CLI #3331
Changes from 26 commits
227365a
80f0661
76273ae
c8f70a1
462ee5f
942ae7f
169fbd1
f6a0eb8
5afefe6
5a6078b
563abbb
0872e94
f9c395f
96dd91f
94ca9d1
50c9fa9
12aee96
4d8fea4
5a48789
6d103ce
662db5a
0b3a9b5
64a999c
1313d54
9371602
4131a0a
c16d843
bcb5df8
d7e407f
b78eacf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 34 in cli/keyring_list.go
Codecov / codecov/patch
cli/keyring_list.go#L33-L34
Check warning on line 39 in cli/keyring_list.go
Codecov / codecov/patch
cli/keyring_list.go#L38-L39
Check warning on line 44 in cli/keyring_list.go
Codecov / codecov/patch
cli/keyring_list.go#L42-L44
Check warning on line 72 in keyring/file.go
Codecov / codecov/patch
keyring/file.go#L71-L72
Check warning on line 61 in keyring/system.go
Codecov / codecov/patch
keyring/system.go#L57-L61
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.
todo: I was worried you would leave this func like this :) Sorry I should have clarified on discord.
I agree that this is the best choice for now, however I think it should error, not silently return - otherwise the users will go bald from the large amount of head scratching that they'll do when trying to figure out why their added keys never appear in the keyring! (it looks like it is empty when they call list on an os-keyring no?)
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 see the logic behind it throwing an error. My original thoughts were that this function should never actually get called so there was no need to have it do anything. But as I think over it some more, that's all the more reason to make it throw an error. I think you are right.
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've made the changes. I'm going to tag you for re-review, so let me know what you think!