Skip to content
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

Merged
merged 30 commits into from
Jan 1, 2025
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
227365a
Issue 3153 first commit
ChrisBQu Dec 4, 2024
80f0661
Fixed one unit test
ChrisBQu Dec 4, 2024
76273ae
Feedback changes
ChrisBQu Dec 5, 2024
c8f70a1
Feedback changes
ChrisBQu Dec 5, 2024
462ee5f
Roll back package-lock.json in playground
ChrisBQu Dec 5, 2024
942ae7f
Adjust white space
ChrisBQu Dec 5, 2024
169fbd1
Removed extraneous line of code
ChrisBQu Dec 5, 2024
f6a0eb8
Major fixes; Changed tests
ChrisBQu Dec 5, 2024
5afefe6
Improve comments, remove error checks
ChrisBQu Dec 5, 2024
5a6078b
Delint
ChrisBQu Dec 5, 2024
563abbb
Merge branch 'sourcenetwork:develop' into issue-3140
ChrisBQu Dec 6, 2024
0872e94
First commit
ChrisBQu Dec 6, 2024
f9c395f
New approach: checking IsDevMode flag in handler_extras.go/Purge func…
ChrisBQu Dec 6, 2024
96dd91f
helpful comment
ChrisBQu Dec 6, 2024
94ca9d1
Delint
ChrisBQu Dec 6, 2024
50c9fa9
Clean up white space and comments
ChrisBQu Dec 6, 2024
12aee96
Changed comment
ChrisBQu Dec 6, 2024
4d8fea4
Adjusted tests
ChrisBQu Dec 6, 2024
5a48789
Merge branch 'issue-2756' of github.com:ChrisBQu/defradb into issue-3140
ChrisBQu Dec 16, 2024
6d103ce
First commit
ChrisBQu Dec 16, 2024
662db5a
Removed changes to unrelated file
ChrisBQu Dec 16, 2024
0b3a9b5
Refactored List() function, updated comments
ChrisBQu Dec 16, 2024
64a999c
Unit test and delint/gofmt
ChrisBQu Dec 16, 2024
1313d54
Updated comments, print statements
ChrisBQu Dec 17, 2024
9371602
Make docs
ChrisBQu Dec 17, 2024
4131a0a
Typo fix
ChrisBQu Dec 18, 2024
c16d843
Error output to systemKeyring.List and unit test
ChrisBQu Dec 18, 2024
bcb5df8
Comment formatting// assert -> require
ChrisBQu Dec 20, 2024
d7e407f
Merge branch 'develop' into issue-2756
ChrisBQu Dec 20, 2024
b78eacf
Merge branch 'develop' into issue-2756
ChrisBQu Jan 1, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ func NewDefraCommand() *cobra.Command {
MakeKeyringGenerateCommand(),
MakeKeyringImportCommand(),
MakeKeyringExportCommand(),
MakeKeyringListCommand(),
)

identity := MakeIdentityCommand()
Expand Down
44 changes: 44 additions & 0 deletions cli/keyring_list.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package cli
ChrisBQu marked this conversation as resolved.
Show resolved Hide resolved

import (
"github.com/spf13/cobra"
)

// MakeKeyringListCommand creates a new command to list all keys in the keyring.
func MakeKeyringListCommand() *cobra.Command {
var cmd = &cobra.Command{
Use: "list",
Short: "List all keys in the keyring",
Long: `List all keys in the keyring.
The DEFRA_KEYRING_SECRET environment variable must be set to unlock the keyring.
This can also be done with a .env file in the working directory or at a path
defined with the --secret-file flag.

Example:
defradb keyring list`,
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
keyring, err := openKeyring(cmd)
if err != nil {
return err
}

Check warning on line 24 in cli/keyring_list.go

View check run for this annotation

Codecov / codecov/patch

cli/keyring_list.go#L21-L24

Added lines #L21 - L24 were not covered by tests

keyNames, err := keyring.List()
if err != nil {
return err
}

Check warning on line 29 in cli/keyring_list.go

View check run for this annotation

Codecov / codecov/patch

cli/keyring_list.go#L26-L29

Added lines #L26 - L29 were not covered by tests

if len(keyNames) == 0 {
cmd.Println("No keys found in the keyring.")
return nil
}

Check warning on line 34 in cli/keyring_list.go

View check run for this annotation

Codecov / codecov/patch

cli/keyring_list.go#L31-L34

Added lines #L31 - L34 were not covered by tests

cmd.Println("Keys in the keyring:")
for _, keyName := range keyNames {
cmd.Println("- " + keyName)
}
return nil

Check warning on line 40 in cli/keyring_list.go

View check run for this annotation

Codecov / codecov/patch

cli/keyring_list.go#L36-L40

Added lines #L36 - L40 were not covered by tests
},
}
return cmd
}
18 changes: 18 additions & 0 deletions keyring/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,21 @@
}
return err
}

func (f *fileKeyring) List() ([]string, error) {

Check failure on line 68 in keyring/file.go

View workflow job for this annotation

GitHub Actions / Lint GoLang job

unnecessary leading newline (whitespace)

files, err := os.ReadDir(f.dir)
if err != nil {
return nil, err
}

Check warning on line 73 in keyring/file.go

View check run for this annotation

Codecov / codecov/patch

keyring/file.go#L68-L73

Added lines #L68 - L73 were not covered by tests

// File names are key names
var keyNames []string
for _, file := range files {
if !file.IsDir() {
keyNames = append(keyNames, file.Name())
}

Check warning on line 80 in keyring/file.go

View check run for this annotation

Codecov / codecov/patch

keyring/file.go#L76-L80

Added lines #L76 - L80 were not covered by tests
}

return keyNames, nil

Check warning on line 83 in keyring/file.go

View check run for this annotation

Codecov / codecov/patch

keyring/file.go#L83

Added line #L83 was not covered by tests
}
2 changes: 2 additions & 0 deletions keyring/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,6 @@ type Keyring interface {
//
// If a key with that name does not exist `ErrNotFound` is returned.
Delete(name string) error
// List returns a list of all keys in the keyring, used by the CLI 'keyring list' command
List() ([]string, error)
}
6 changes: 6 additions & 0 deletions keyring/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,9 @@
func (s *systemKeyring) Delete(user string) error {
return keyring.Delete(s.service, user)
}

func (s *systemKeyring) List() ([]string, error) {
// List does not need to be returned here
// This function is a stub
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

return nil, nil

Check warning on line 60 in keyring/system.go

View check run for this annotation

Codecov / codecov/patch

keyring/system.go#L57-L60

Added lines #L57 - L60 were not covered by tests
Copy link
Contributor

@AndrewSisley AndrewSisley Dec 18, 2024

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?)

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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!

}
Loading