-
Notifications
You must be signed in to change notification settings - Fork 51
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
Conversation
Please add the |
keyring/system.go
Outdated
// List does not need to be returned here | ||
// This function is a stub |
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.
suggestion: It would be nice to have a bit more details as to why list does not need to be returned for the system keyring.
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 seems like a fair suggestion. I agree with it. I've added a bit more to it.
I'm not sure how much detail you (or other team members(?)) think this comment should go into. I'm open to discussion on this point if anyone feels strongly.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3331 +/- ##
===========================================
+ Coverage 77.92% 78.03% +0.11%
===========================================
Files 388 389 +1
Lines 35398 35446 +48
===========================================
+ Hits 27582 27660 +78
+ Misses 6177 6143 -34
- Partials 1639 1643 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
keyring/system.go
Outdated
// The OS keyring does not support listing keys | ||
// This function is a stub for now because the Keyring interface requires it | ||
// Currently, the 'defradb keyring list' command uses only fileKeyring | ||
return nil, nil |
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!
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 would request changes, but am on holiday and don't want to block the PR. Please address/push-back my one todo before merge :)
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.
LGTM. Just a couple comments that you can resolve if you want to.
keyring/errors.go
Outdated
) | ||
|
||
var ( | ||
ErrNotFound = keyring.ErrNotFound // ErrNotFound is returned when a keyring item is not found. |
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.
nitpick: Not a fan of descriptive comments more than a handful of words being located on the same line as code.
keyring/system_keyring_list_test.go
Outdated
assert.Nil(t, keys, "keys should be nil when List is called") | ||
assert.Error(t, err, "an error should be returned when List is called") | ||
assert.Equal(t, ErrSystemKeyringListInvoked, err, "the error should be ErrSystemKeyringListInvoked") |
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.
suggestion: We tend to prefer using require
instead of assert
but regardless of which one you chose, you could replace assert.Error
and assert.Equal
with assert.ErrorIs
(or require.ErrorIs
).
assert.ErrorIs(t, err, ErrSystemKeyringListInvoked)
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.
Still a bit of growing pains getting used to writing Go! But I agree with this suggestion (both parts.)
Making this change.
Relevant issue(s)
Resolves #2756
Description
I am adding support for a new feature in the CLI. By running
defradb keyring list
, the names of the keys in the fileKeyring will be listed out.Tasks
How has this been tested?
Specify the platform(s) on which this was tested: