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

added key_gen function to client #86

Merged
merged 3 commits into from
Jun 1, 2017
Merged

Conversation

sunnya97
Copy link
Contributor

Useful for creating new keys in order to push multiple objects to the IPNS namespace using name_publish.

Copy link
Contributor

@ntninja ntninja left a comment

Choose a reason for hiding this comment

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

Looking good except for a few minor things! Could you also add the missing key_list method while you're at it, so that we have feature-parity with the command-line client for this again?

@@ -743,6 +743,42 @@ def resolve(self, name, recursive=False, **kwargs):
args = (name,)
return self._client.request('/resolve', args, decoder='json', **kwargs)

def key_gen(self, keyName, type='rsa', size='2048', **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make type a mandatory argument (we don't want to hard-code a crypto type that may eventually become obsolete) and size an int.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please write key_name instead of keyName – we use snake_case everywhere else, no point in introducing any inconsistency here.

keyName : str
Name of the new Key to be generated. Used to reference the Keys.
type : str
Type of key to generate
Copy link
Contributor

Choose a reason for hiding this comment

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

Please list the currently possible key types (rsa, ed25519) here so that they appear in the API documentation

dict : Key name and Key Id
"""

existingKeys = self._client.request('/key/list', decoder='json')['Keys']
Copy link
Contributor

Choose a reason for hiding this comment

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

See the above comment about snake_case.

Copy link
Contributor

@ntninja ntninja left a comment

Choose a reason for hiding this comment

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

Thanks! Looking good now 🙂
I'll merge this in the next couple of days, after (hopefully) figuring out way the CI is broken…

@whereswaldon
Copy link
Contributor

@Alexander255 The problem here seems to be a couple of style errors and a functional test that crashes the running ipfs node. That could be resolved by using the later version of IPFS though, so it may be worth rebasing on top of master?
@sunnya97 Thanks for working on this!

@ntninja
Copy link
Contributor

ntninja commented May 30, 2017

Yes, @sunnya97, please rebase onto master… most of the CI errors should now be fixed.

Also please run tox -e pep8 fix the errors reported by the style-checker (I know that thing is quite annoying, sorry 'bout that) before pushing again, so that the build won't fail because of that.

Thanks so much for helping us keeping the library updated! 😀

@sunnya97
Copy link
Contributor Author

Updated!

Copy link
Contributor

@ntninja ntninja left a comment

Choose a reason for hiding this comment

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

Still looking good and the CI also works again! Thanks! 🙂

@ntninja ntninja merged commit fe8c417 into ipfs-shipyard:master Jun 1, 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.

3 participants