-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Move key rotation command to ipfs key rotate #7599
Conversation
aa93ab1
to
d8ad119
Compare
3307a47
to
5821766
Compare
5821766
to
0f65848
Compare
if !ok { | ||
return fmt.Errorf("keystore name for backing up old key must be provided") | ||
} | ||
return doRotate(os.Stdout, cctx.ConfigRoot, oldKey, algorithm, nBitsForKeypair, nBitsGiven) |
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'm using Writer's here and Stdout to mimic what we do in ipfs init
. We could potentially return objects like we do for other commands here, but the user could just call ipfs id
to get the new ID, so there's not much lost.
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.
Seems reasonable. If it's an issue later, we can always fix this to make --enc=json
work properly.
cmds.StringOption(keyStoreTypeOptionName, "t", "type of the key to create: rsa, ed25519").WithDefault(keyStoreAlgorithmDefault), | ||
cmds.IntOption(keyStoreSizeOptionName, "s", "size of the key to generate"), |
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.
Not sure what I should do here for the option names. Previously, type, t
and size, s
were copied from ipfs init
and used algorithm, a
and bits, b
. It seems to me like keeping these names consistent with ipfs key gen
is the most reasonable.
We could also just add more aliases (e.g. type, t, algorithm, a
), but this seems excessive for a new command. Thoughts?
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 agree. I'd just do what ipfs key gen
does and break things (we haven't cut a release yet anyways).
@@ -146,7 +146,7 @@ func genKeys(t *testing.T, keyType int) (ci.PrivKey, peer.ID, string, string) { | |||
return sk, id, PkKeyForID(id), ipns.RecordKey(id) | |||
} | |||
|
|||
func createIPNSRecordWithEmbeddedPublicKey(sk ci.PrivKey, val []byte, seq uint64, eol time.Time) (*ipns_pb.IpnsEntry, error){ | |||
func createIPNSRecordWithEmbeddedPublicKey(sk ci.PrivKey, val []byte, seq uint64, eol time.Time) (*ipns_pb.IpnsEntry, error) { |
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.
go fmt that was ignored in a previous commit... my bad. Need to get linting rules better set up.
cmds.IntOption(keyStoreSizeOptionName, "s", "size of the key to generate"), | ||
}, | ||
NoRemote: true, | ||
PreRun: func(req *cmds.Request, env cmds.Environment) error { |
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.
nit: I'm not sure if this is actually necessary. Opening the repo below should take care of this.
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
No description provided.