Nexmo / nexmo-cli

Nexmo CLI (Command Line Interface)
https://nexmo.com
MIT License
78 stars 52 forks source link

Applications v2 support for nexmo-cli@beta #205

Closed AlexLakatos closed 4 years ago

AlexLakatos commented 4 years ago

Summary

Added Apps v2 to the CLI

Other Information

To test:

npm install -g nexmo-cli@alpha

nexmo as asdasdas-asdd-2344-2344-asdasdasd345 --v2 nexmo as asdasdas-asdd-2344-2344-asdasdasd345 --recreate

nexmo app:update asdasdas-asdd-2344-2344-asdasdasd345

nexmo app:create

nexmo jwt:generate

For the new public key flag, to generate a public/private key pair first:

ssh-keygen -t rsa -b 4096 -m PEM -f private.key
# Don't add passphrase
openssl rsa -in private.key -pubout -outform PEM -out public.key.pub
lornajane commented 4 years ago

I'm a bit confused about required/optional arguments. I started with supplying just the app name, and that created me an app (with no config, which is what I expected). When I created with an answer_url, the answer URL doesn't show in app:show. My command is:

nexmo app:create "TEST1" --keyfile private.key --voice-answer-url=https://somewhere.ngrok.io

lornajane commented 4 years ago

Only slightly related: can we do something with the help output? We have too many commands now (in a good way)!!

AlexLakatos commented 4 years ago

That command works with Applications V1. And should give you a voice application with null urls. To use V2, you need to specify --capabilities, and that makes the capability specific flags required.

AlexLakatos commented 4 years ago

I can't do anything about the help output sadly. commander is what it is.

pardel commented 4 years ago

Feature request: could we also have the full path to the private.key file? (similar to the credentials)

tbedford commented 4 years ago

@AlexLakatos: For openssl rsa -in jwtRS256.key -pubout -outform PEM -out public.key.pub should that actually be openssl rsa -in private.key -pubout -outform PEM -out public.key.pub?

AlexLakatos commented 4 years ago

yes, forgot to change the file name in the command

On Fri, Sep 20, 2019, 18:36 Tony Bedford notifications@github.com wrote:

@AlexLakatos https://github.com/AlexLakatos: For openssl rsa -in jwtRS256.key -pubout -outform PEM -out public.key.pub should that actually be openssl rsa -in private.key -pubout -outform PEM -out public.key.pub?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Nexmo/nexmo-cli/pull/205?email_source=notifications&email_token=AAFIPVGRX6XYR542XQ2AVTDQKSRTXA5CNFSM4IYJIQ22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7GJQSQ#issuecomment-533502026, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFIPVCMFFVG6HKJWHM6WCTQKSRTXANCNFSM4IYJIQ2Q .

AlexLakatos commented 4 years ago

@lornajane wanted a "show create command" for an existing application

@sammachin wanted interactive mode to display the full command line setting before it runs, so you can see the options and if you want to repeat just copy/paste the config

tbedford commented 4 years ago

For this:

abedford@Bilbo: ~ [] $ nexmo app:show 123  --recreate
/Users/abedford/node-v10/lib/node_modules/nexmo-cli/lib/emitter.js:179
  var entries = Object.keys(data).map(function (key) {
                       ^

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at formatList (/Users/abedford/node-v10/lib/node_modules/nexmo-cli/lib/emitter.js:179:24)
    at Emitter.list (/Users/abedford/node-v10/lib/node_modules/nexmo-cli/lib/emitter.js:104:24)
    at Response.<anonymous> (/Users/abedford/node-v10/lib/node_modules/nexmo-cli/lib/response.js:271:25)
    at HttpClient.__parseResponse (/Users/abedford/node-v10/lib/node_modules/nexmo-cli/node_modules/nexmo/lib/HttpClient.js:232:9)
    at IncomingMessage.<anonymous> (/Users/abedford/node-v10/lib/node_modules/nexmo-cli/node_modules/nexmo/lib/HttpClient.js:141:19)
    at IncomingMessage.emit (events.js:203:15)
    at endReadableNT (_stream_readable.js:1129:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

Would expect something like app id not found or app does not exist.

AlexLakatos commented 4 years ago

nice catch @tbedford. fixed it now in the new alpha-6, please re-install the cli

tbedford commented 4 years ago

In interactive mode, after the application is created, the message displayed is:

"Run this command again without interactive mode:"

This is slightly confusing as it seems to imply you have to run this command to actually create the application (which you don't it's already created).

New wording might be along the lines of "To recreate this application in the future without interactive mode use the following command:".

AlexLakatos commented 4 years ago

@tbedford thanks for that, the messages do need some work. Can you also take a look at the app:show --recreate message, and suggest something better there as well?

lornajane commented 4 years ago

Definitely good to get @tbedford on the wording, it could be better and his input will be excellent.

This is a lot of interactive questions now. For example, how about "filename for private key [leave blank to have it in command output and not saved]" in one question (again, maybe better wording)? Can we skip optional things like the fallback url or something, to get people more likely to make it out alive? Is that just me?

AlexLakatos commented 4 years ago

@lornajane I am worried we're not being verbose enough here, and we'll get support queries for this.

As for the optional things, I don't have a good idea on how to re-do the interactive mode with "minimal" and "all" available options. I think the default for the interactive CLIs I'm using is "all" the options, the expectation is that I don't run through the interactive mode on a regular basis, it's usually a one-off and then I'll use the non-interactive command.

AlexLakatos commented 4 years ago

@pardel added the jwt:generate with local app settings as you wanted.

@lornajane managed to cut down on the questions like you wanted for the keys with "leave empty if". And added defaults as well(example.com for create, and the initial app properties for update), so now you can "Enter" your way to the end of the prompts.

lornajane commented 4 years ago

Found a tiny bug:

$ nexmo apps:list -vvv
Item 1-NaN of undefined

I'm confused about when the interactive thing should occur. I tried to update one field by checking the usage information for app:update and then running

$ nexmo app:update 498d365c-2fca-4906-ba49-012c29e5f1b2 --messages-status-url https://ljnexmo.eu.ngrok.io/webhooks/message-status

However this then set off the interactive thing and it re-asked all the questions, which I didn't expect.

I accidentally updated the .nexmo-app file so I tried to set it back to the application I really want to work with. The private.key file relates to the application ID I want to use but I can't seem to supply the filename or work out how to safely paste a private key onto the commandline. Any advice on that?

lornajane commented 4 years ago

The reduced number of questions and intelligent defaults is really nice, I think this is pretty close to be honest. I do wish the default was to save to private.key though! I'm nitpicking, so you know that it's actually awesome.

I'll take a look at the code as well but I'm not sure when ...

AlexLakatos commented 4 years ago

I'm confused about when the interactive thing should occur. I tried to update one field by checking the usage information for app:update and then running

$ nexmo app:update 498d365c-2fca-4906-ba49-012c29e5f1b2 --messages-status-url https://ljnexmo.eu.ngrok.io/webhooks/message-status

Interactive mode happens when a name is not supplied. As for partial app updates, I wish our API supported that but sadly it's all or nothing, the app gets replaces with whatever you put in the request.

AlexLakatos commented 4 years ago

I accidentally updated the .nexmo-app file so I tried to set it back to the application I really want to work with. The private.key file relates to the application ID I want to use but I can't seem to supply the filename or work out how to safely paste a private key onto the commandline. Any advice on that?

nexmo app:setup app_id path/to/private/key works for that, and the .nexmo-app will have the path to private.key instead of the actual private key. The CLI commands still work if there is a path there though. Or they should. I just tested it out and it works:

@lakatos88 > ~/nexmo/nexmo-demo/cli-tests $ nexmo app:setup 2787eb58-c010-4042-bca7-46619757c221 private.key
Credentials written to /Users/laka/nexmo/nexmo-demo/cli-tests/.nexmo-app
@lakatos88 > ~/nexmo/nexmo-demo/cli-tests $ cat .nexmo-app 
[app_config]
app_id=2787eb58-c010-4042-bca7-46619757c221
private_key=private.key
@lakatos88 > ~/nexmo/nexmo-demo/cli-tests $ nexmo cc display_name=testy
Conversation created: CON-de48f280-1240-4964-82ae-478030495cfd
AlexLakatos commented 4 years ago

I do wish the default was to save to private.key though!

But then we'd need something else for "don't save" there, and it becomes more complicated.

pardel commented 4 years ago

Have we ever considered storing these keys in a central ~/.nexmo folder?

AlexLakatos commented 4 years ago

After running some of the commands, I'm getting issues where the CLI expects a fallback URL, but none of my apps have one supplied. @AlexLakatos is aware and will take care of, and I'll make a new app via the CLI to continue testing.

Nice catch @dragonmantank. Fixed it in the new alpha, so you'll want to re-install the cli.

lornajane commented 4 years ago

This is totally coming together, great work @AlexLakatos :trophy:

With the applications, do they "know" if they are a v1 or a v2 application? I keep forgetting I need the --v2 switch for my applications now - and we will always need this in the future. Can we detect what sort of app we have and output accordingly?

Also I think if the --recreate switch is given then we could probably output only the command.

lornajane commented 4 years ago

Can we check that the public key IS a public key? If it starts -----BEGIN RSA PRIVATE KEY----- can we point out that this is the wrong file?

lornajane commented 4 years ago

Are we intending to allow edit or delete of users or conversations here? I think it would be useful to allow listing of both of those so it's clear what IDs to use to add members to conversations

AlexLakatos commented 4 years ago

This is totally coming together, great work @AlexLakatos 🏆

With the applications, do they "know" if they are a v1 or a v2 application? I keep forgetting I need the --v2 switch for my applications now - and we will always need this in the future. Can we detect what sort of app we have and output accordingly?

Also I think if the --recreate switch is given then we could probably output only the command.

V2 sees V1 applications and automatically converts V1 types into V2 capabilities. The only thing that differs is the response format, we're always using V2 API, just massaging the response sometimes to output a V1 like response

AlexLakatos commented 4 years ago

Can we check that the public key IS a public key? If it starts -----BEGIN RSA PRIVATE KEY----- can we point out that this is the wrong file?

The API should do that for us, I didn't think it was worth doing in the cli as well. But I can be swayed if you feel strongly about it.

AlexLakatos commented 4 years ago

Are we intending to allow edit or delete of users or conversations here? I think it would be useful to allow listing of both of those so it's clear what IDs to use to add members to conversations

That's on a different to-do list for after Conversations V2 lands. I'm trying to keep this PR all about Applications V2, even though I'd doing a poor job of it.

lornajane commented 4 years ago

OK so conversations API should be out of scope (actually I think I'm seeing things that were already in the beta, rather than in this PR), let's try to ignore those for now.

I'm still concerned about needing to type --v2 for evermore even though that is our new default/preferred format. If I created an application with this tool, and it created a v2 application, then I should be able to show the details and get it as a v2 by default without the extra switch. Is that even possible?

pardel commented 4 years ago

@AlexLakatos Question re: private key option: "? Private Key path: Leave empty if you don't want to save your private key."

Is this default good? Feels like the user won't be able to do much from the cli without a private key.

mheap commented 4 years ago

The existing behaviour outputs it to stdout rather than saving. If commands now also read from .nexmo-app then we don't need it stored in private.key too.

I'd like to see us ass .nexmo-app to .gitignore automatically if it exists. I've seen a lot of interview candidates accidentally committing it to their repos

AlexLakatos commented 4 years ago

@AlexLakatos Question re: private key option: "? Private Key path: Leave empty if you don't want to save your private key."

Is this default good? Feels like the user won't be able to do much from the cli without a private key.

This is the current default for the CLI, nothing's changed. The key still gets saved to .nexmo-app, it just doesn't get saved in a private.key file. Same as we do now, that's why we tell people to do --keyfile for some of our documentation.

You need to keep in mind the primary use of the CLI is not for power users like our team, that generate a new app every other day. And some teams use it in deployment scripts rather than live, they don't want to be saving the keyfile on disk.

pardel commented 4 years ago

Should we require at least one capability when creating an app?

Screenshot 2019-09-30 at 12 51 59
AlexLakatos commented 4 years ago

I'm still concerned about needing to type --v2 for evermore even though that is our new default/preferred format. If I created an application with this tool, and it created a v2 application, then I should be able to show the details and get it as a v2 by default without the extra switch. Is that even possible?

Should we require at least one capability when creating an app?

The API doesn't, so why should the CLI?

pardel commented 4 years ago

Should we require at least one capability when creating an app?

The API doesn't, so why should the CLI?

Good question. My understanding of a CLI is to provide a nicer experience than calling the APIs directly. But then again, mobile developers don't used CLIs much 😄

AlexLakatos commented 4 years ago

I'm still concerned about needing to type --v2 for evermore even though that is our new default/preferred format. If I created an application with this tool, and it created a v2 application, then I should be able to show the details and get it as a v2 by default without the extra switch. Is that even possible?

Switching the default would be a breaking change and would mean having to update to a major CLI version for beta only, and I don't think we should do that. When the node SDK is moving out of beta, the V2 will become default, --v2 will go away, and we'll do a major CLI release.

AlexLakatos commented 4 years ago

Should we require at least one capability when creating an app?

The API doesn't, so why should the CLI?

Good question. My understanding of a CLI is to provide a nicer experience than calling the APIs directly. But then again, mobile developers don't used CLIs much 😄

I don't think we should limit the CLI, if the API supports it.

Now if we think the API is broken and it should not support it, that's something we should fix in the API, and not patch in the tooling, especially since the CLI is about providing an alternative experience to the management side of things, and not to provide a nicer experience than calling the API. For that last case, we have the server SDKs.

pardel commented 4 years ago

Should we provide a warning if nexmo app:create is about to overwrite a .nexmo-app in the current folder?

pardel commented 4 years ago

Feels like the "Private Key path" prompt is redundant when public key is provided:

Screenshot 2019-09-30 at 13 15 21
AlexLakatos commented 4 years ago

Feels like the "Private Key path" prompt is redundant when public key is provided:

It is, I forgot to make them depend on eachother in the prompt, good catch!

pardel commented 4 years ago

A missing public key throws an exception:

Screenshot 2019-09-30 at 13 22 15

(sorry for posting a screenshot - code blocks don't seem to respect new lines)

AlexLakatos commented 4 years ago

A missing public key throws an exception:

Yes, it should. Are you saying I should hide the stacktrace instead?

dragonmantank commented 4 years ago

After playing around with this as well as looking over the code, looks good to me 👍

AlexLakatos commented 4 years ago

Thanks for all the feedback.

I've just fixed the issues with private / public key @pardel brought up. And made empty capabilities apps possible.

With this, if the tests pass, I'll merge and release.

If there is something from the feedback that you feel I didn't address, or if you have more ideas for the CLI, please file an issue and I'll address those post Campus.