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

Implement ipfs key {rm, rename} #3892

Merged
merged 10 commits into from May 18, 2017

Conversation

Projects
None yet
3 participants
@MichaelMure
Copy link
Contributor

MichaelMure commented May 1, 2017

@whyrusleeping So you don't waste time before implementing the good encryption stuff ;)

License: MIT
Signed-off-by: Michael Muré batolettre@gmail.com

Implement ipfs key rm
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>
Tagline: "Remove a keypair",
},
Arguments: []cmds.Argument{
cmds.StringArg("name", true, false, "name of key to remove"),

This comment has been minimized.

@Kubuxu

Kubuxu May 1, 2017

Member

This command should allow for StdIn handling and multiple argument handling. Then it would be possible to remove multiple keys at the same time easily.

@Kubuxu

This comment has been minimized.

Copy link
Member

Kubuxu commented May 1, 2017

See also codeclimate errors. No need to export this command definition. We are switching to not exporting them where not needed.

MichaelMure added some commits May 1, 2017

fix fmt and exported var that shouldn't be
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>
Make ipfs key rm variadic
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>

@MichaelMure MichaelMure force-pushed the MichaelMure:keystore_key_rm branch from 469e7db to bd96f71 May 1, 2017

Tagline: "Remove a keypair",
},
Arguments: []cmds.Argument{
cmds.StringArg("name", true, true, "names of keys to remove"),

This comment has been minimized.

@Kubuxu

Kubuxu May 1, 2017

Member

Call on the return value of StringArg .EnableStdin() to enable stdin handling for it.

MichaelMure added some commits May 2, 2017

Enable stdin for ipfs key rm's name argument
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>
Add a Has(name) method to the keystore
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>
Implement ipfs key rename
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>
add sharness test for ipfs key rename
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>

@MichaelMure MichaelMure changed the title Implement ipfs key rm Implement ipfs key {rm, rename} May 4, 2017

@MichaelMure

This comment has been minimized.

Copy link
Contributor Author

MichaelMure commented May 4, 2017

@Kubuxu @whyrusleeping I added an implementation of ipfs key rename, because why not.

@whyrusleeping
Copy link
Member

whyrusleeping left a comment

One request on the implementation of Has, otherwise LGTM, very clean code :)

func (ks *FSKeystore) Has(name string) bool {
kp := filepath.Join(ks.dir, name)

_, err := os.Stat(kp)

This comment has been minimized.

@whyrusleeping

whyrusleeping May 5, 2017

Member

Has should probably also return an error. If we run into some permissions issue, this will report that the key doesnt exist. use os.IsErrNotExist to check for file existence, otherwise the error is a real error

Future-proof keystore.Has by returning an error as well
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>
@whyrusleeping

This comment has been minimized.

Copy link
Member

whyrusleeping commented May 13, 2017

I would add a test that makes sure you cant rename self to something else. Then codeclimate has three issues to fix.

After that, good to merge

@whyrusleeping whyrusleeping added this to the Ipfs 0.4.10 milestone May 13, 2017

@Kubuxu

Kubuxu approved these changes May 13, 2017

@Kubuxu

Kubuxu approved these changes May 13, 2017

Copy link
Member

Kubuxu left a comment

after what @whyrusleeping pointed out

(Regarding CodeClimate, we introduced this requirement recently to incrementally improve our codebase).

MichaelMure added some commits May 14, 2017

Add a test for failure of 'ipfs key rm self'
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>
Document exported symbols
License: MIT
Signed-off-by: Michael Muré <batolettre@gmail.com>
@MichaelMure

This comment has been minimized.

Copy link
Contributor Author

MichaelMure commented May 16, 2017

@whyrusleeping It should be good now, can you merge ?

@whyrusleeping whyrusleeping merged commit 0780a4f into ipfs:master May 18, 2017

7 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate 10 fixed issues
Details
codecov/patch 54.08% of diff hit (target 35.22%)
Details
codecov/project 62.93% (+27.71%) compared to f44ff30
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@whyrusleeping

This comment has been minimized.

Copy link
Member

whyrusleeping commented May 18, 2017

Thanks @MichaelMure :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment