feat(credentials): add credentials list command #375
Conversation
|
Thank you so much, you read my mind, I was in need to list the credentials created ⭐️ Looking for this one as I'm finalizing my next demo with porter (and yes, I did stay in the CNAB context this time 😈 ) |
| @@ -9,7 +9,7 @@ import ( | |||
| func buildCredentialsCommand(p *porter.Porter) *cobra.Command { | |||
| cmd := &cobra.Command{ | |||
| Use: "credentials", | |||
| Aliases: []string{"cred"}, | |||
| Aliases: []string{"credential", "cred", "creds"}, | |||
cmd/porter/credentials.go
Outdated
| Short: "List credentials", | ||
| Long: `List credentials available to Porter. | ||
|
|
||
| A listing of credentials currently available to Porter will be provided, along with metadata such as modification time, etc. |
There was a problem hiding this comment.
I think we don't need to tell them up-front what columns are going to be in the table. I'd just leave it at "List named sets of credentials defined by the user" and leave the rest off.
The part about the table outputs is already in the --output flag, and doesn't need to be repeated.
| rawFormat string | ||
| format printer.Format | ||
| }{} | ||
| opts := porter.ListOptions{} |
pkg/porter/credentials.go
Outdated
| // TODO: should this go somewhere else? | ||
| // i.e., where should logic for init-ing dirs needed by porter go? | ||
| // e.g., on 'porter creds list' with a fresh porter install/home | ||
| if ok, _ := p.Context.FileSystem.DirExists(dir); !ok { |
There was a problem hiding this comment.
I would prefer that a readonly command list not have a side-effect, creating a directory. One way to deal with this a bit easier is to have a function that does the walk and returns a slice of fileinfo's and a error. Similar to how we have a function that gets the schema and another function that prints the schema. The retrieval isn't tied to the printing.
Then we could return an empty list when the directory doesn't exist.
What do you think?
There was a problem hiding this comment.
If it's not clear, I'm suggesting that we break this function up into 2 functions. One that traverses the credentials directory that builds up a slice of the credentialsets and another than does the printing. 😀
There was a problem hiding this comment.
Good call; split into two functions. Lemme know how this looks when convenient.
pkg/porter/credentials.go
Outdated
| credSet := &credentials.CredentialSet{} | ||
| data, err := p.Context.FileSystem.ReadFile(path) | ||
| if err != nil { | ||
| return errors.Wrap(err, fmt.Sprintf("unable to load credential set from %s:\n%s", path, err)) |
There was a problem hiding this comment.
This is double printing the error err. The line should be
return errors.Wrapf(err, "unable to load credential set from %s", path)
pkg/porter/credentials.go
Outdated
| return errors.Wrap(err, fmt.Sprintf("unable to load credential set from %s:\n%s", path, err)) | ||
| } | ||
| if err = yaml.Unmarshal(data, credSet); err != nil { | ||
| return errors.Wrap(err, "unable to unmarshal credential set") |
There was a problem hiding this comment.
This error isn't saying which file can't be unmarshaled. I'd change it to include the credential set name in the error message.
There was a problem hiding this comment.
If unmarshal-ing fails, credSet.Name never gets populated and so we don't have a name to print. I've updated to print filepath in this case -- what do you think?
There was a problem hiding this comment.
We can print the name of the file, which is the same as credSet.Name.
pkg/porter/credentials.go
Outdated
| return nil | ||
| }) | ||
| if err != nil { | ||
| return &CredentialsFileList{}, errors.Wrap(err, "encountered error while listing credentials") |
There was a problem hiding this comment.
It would be really frustrating if there was a problem with a single credential, and that preventing anything from being printed out.
I would lean towards (maybe in a follow up PR), is when it's an error reading or unmarshaling a credential, print the error to p.Err, potentially wrapped behind a p.Debug check. That way they could see the see the problem, and filtering it using 2>, and not have that prevent them from seeing their other credentials.
There was a problem hiding this comment.
Good point. I've updated the logic to log to p.Err if debug is set for both unmarshal and ReadFile failures and return nil in both cases, such that listing may proceed. Added a test as well. How does this look?
vdice
force-pushed
the
feat/creds-ls
branch
2 times, most recently
from
570ecc8
to
3afd7bc
Compare
credentialstocredentialsgeneratorto avoid naming confusion with duffle's/cnab-go'scredentialspkg. ping @jeremyrickard on this change for thoughts, etc.A few TODOs remain, but I wanted to get early feedback on this one.
(Note: planning on tackling the table printing revamp TODO either in #371 or a separate ticket, as this will affect multiple pre-existing commands.)
Closes #366