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

"ipfs key list": include the hash of the key id in addition to the name #3581

Merged
merged 3 commits into from
Jan 11, 2017

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Jan 10, 2017

First stab, closes #3568

License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
@kevina kevina added the status/in-progress In progress label Jan 10, 2017
@kevina
Copy link
Contributor Author

kevina commented Jan 10, 2017

@whyrusleeping not sure about this at all, feedback welcome, fell free to just take over this pr if it will be faster.

return
}

list = append(list, key+" "+pid.Pretty())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an array of strings here will make the json output of the api hard to parse. Lets make the output be an array of KeyOutput structs (which probably means defining a KeyOutputList struct because our use of the type system in the commands lib is weird).

Lets add a -l or --show-ids flag to the command and then in the output marshaler, then check that for whether or not to print the keys alongside the names.
Then in the text output marshaler, use a tabwriter to make sure the columns are aligned evenly when outputting both names and keys.

@whyrusleeping
Copy link
Member

CHANGELOG:
Add key ID output option to 'ipfs key list' command.

@kevina
Copy link
Contributor Author

kevina commented Jan 10, 2017

@whyrusleeping do you think it would be better for the hash to come first. It would make it easier to parse by shell utilities if the key name has spaces in it and would also be more consistent with "ipfs ls". I don't have a strong preference either way.

License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
@kevina
Copy link
Contributor Author

kevina commented Jan 11, 2017

Done. I listed the hash first when the "-l" option is used, but can easily swap them.

@@ -135,6 +142,9 @@ var KeyListCmd = &cmds.Command{
Helptext: cmds.HelpText{
Tagline: "List all local keypairs",
},
Options: []cmds.Option{
cmds.BoolOption("show-ids", "l", "also show key ids"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say just l, an have it catch all long format output, I think we might have more in future.

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@whyrusleeping whyrusleeping added this to the ipfs 0.4.5 milestone Jan 11, 2017
@kevina
Copy link
Contributor Author

kevina commented Jan 11, 2017

@whyrusleeping your changes look good to me. Merge when ready. Let me know if there is anything else you need from me on this.

@whyrusleeping whyrusleeping merged commit 3faf897 into master Jan 11, 2017
@whyrusleeping whyrusleeping deleted the kevina/keystore branch January 11, 2017 16:44
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No ( apparent ) way to list the hash of existing keys
3 participants