apache / openwhisk-cli

Apache OpenWhisk Command Line Interface (CLI)
https://openwhisk.apache.org/
Apache License 2.0
103 stars 99 forks source link

"wsk property get" should return raw output when provided with specific property. #430

Closed neerajmangal closed 5 years ago

neerajmangal commented 5 years ago

This PR attempts to address issues explained in #407.

With this PR, wsk CLI will provide a raw output when a specific property is provided in "wsk property get" command.

"wsk property get"

Neerajs-MacBook-Pro:incubator-openwhisk-cli mangal$ ./wsk property get
client cert
Client key
whisk auth      2eafce54-d08e-4128-a109-#####
whisk API host      https://wsk-edge#####.com
whisk API version   v1
whisk namespace     mangal
whisk CLI version   not set
whisk API build     2018-01-29T18:32:59Z
whisk API build number  latest
Neerajs-MacBook-Pro:incubator-openwhisk-cli mangal$
wsk property get --namespace
mangal

# There is no need to parsing the output now. 

export NAMESPACE=`wsk property get --namespace`
echo $NAMESPACE
mangal
wsk property get --apihost
https://wsk-edge.#####.com
wsk property get --auth
2eafce54-d08e-4128-a109-####

If Option "--all" is present with other options then --all takes precedence.

Neerajs-MacBook-Pro:incubator-openwhisk-cli mangal$ ./wsk property get --namespace --all
client cert
Client key
whisk auth      2eafce54-d08e-4128-a109-######
whisk API host      https://wsk-edge.#####.com
whisk API version   v1
whisk namespace     mangal
whisk CLI version   not set
whisk API build     2018-01-29T18:32:59Z
whisk API build number  latest
Neerajs-MacBook-Pro:incubator-openwhisk-cli mangal$
neerajmangal commented 5 years ago

Fixing integration tests as they depend on output "text" of 'wsk property get' command.

mdeuser commented 5 years ago

@neerajmangal - currently, this appears to be a breaking change.. that is, existing scripts will fail when this is merged as-is. what about adding a new flag - that could be used by all commands - to specify the format of the output (i.e. json, raw, etc)

neerajmangal commented 5 years ago

@neerajmangal - currently, this appears to be a breaking change.. that is, existing scripts will fail when this is merged as-is. what about adding a new flag - that could be used by all commands - to specify the format of the output (i.e. json, raw, etc)

Hi @mdeuser , Yes this will possibly break existing scripts if they are using "property get" and then parsing the output.

I initially thought of adding a "--raw" option but let me know what should be the best option in this case. But I believe "-- propertyname" can be think of raw input to the command "wsk property get".

  1. Modify existing scripts to cater to this change and add documentation about it.
  2. Adding a new flag like "--raw" into existing commands.
mdeuser commented 5 years ago

what about a --output | -o flag that takes a single value specifying the desired output type?

./wsk property get --namespace -o json   // for future pr :-)
./wsk property get --namespace -o raw
./wsk property get --namespace -o std    // today's formatting
./wsk property get --namespace           // defaults to "std" formatting

additional output formats can be added without requiring new option flags.

neerajmangal commented 5 years ago

what about a --output | -o flag that takes a single value specifying the desired output type?

./wsk property get --namespace -o json   // for future pr :-)
./wsk property get --namespace -o raw
./wsk property get --namespace -o std    // today's formatting
./wsk property get --namespace           // defaults to "std" formatting

additional output formats can be added without requiring new option flags.

@mdeuser , I attempted an implementation as suggested.

New option --output|-o is added to property get command for now with std|raw.

./wsk property get --help
get property
Usage:
  wsk property get [flags]

Flags:
      --all             all properties
      --apibuild        whisk API build version
      --apibuildno      whisk API build number
      --apihost         whisk API host
      --apiversion      whisk API version
      --auth            authorization key
      --cert            client cert
      --cliversion      whisk CLI version
      --key             client key
      --namespace       whisk namespace
  -o, --output string   Output format in std|raw (default "std")

Global Flags:
  -d, --debug      debug level output
  -i, --insecure   bypass certificate checking
  -v, --verbose    verbose output

default behavior is "std" if no --output|-o option is provided

./wsk property get
client cert
Client key
whisk auth      23bc46b1-71f6-4ed5-8c54-xxxx
whisk API host      https://wsk-edge.xxx.com
whisk API version   v1
whisk namespace     _
whisk CLI version   2019-03-27T20:19:17.662+0530
whisk API build     2018-01-29T18:32:59Z
whisk API build number  latest

./wsk property get --all
client cert
Client key
whisk auth      23bc46b1-71f6-4ed5-8c54-xxxx
whisk API host      https://wsk-edge.xxx.com
whisk API version   v1
whisk namespace     _
whisk CLI version   2019-03-27T20:19:17.662+0530
whisk API build     2018-01-29T18:32:59Z
whisk API build number  latest

./wsk property get --namespace
whisk namespace     _

./wsk property get --apihost
whisk API host      https://wsk-edge.xxxxxx.com

With raw Option

./wsk property get --apihost --output raw
https://wsk-edge.corp.adobe.com

./wsk property get --namespace -o raw
_

If "raw" is provided with --all or without property it will return an error.

./wsk property get --output raw
error: --output|-o raw only supported with specific property type

./wsk property get --all --output raw
error: --output|-o raw only supported with specific property type

check for bugus output type.

./wsk property get --namespace -o jalskkfla
error: Supported output format are std|raw

if "std" provided with --all or without a specific property, cmd will treat it as default behavior.

/wsk property get --all -o std
client cert
Client key
whisk auth      23bc46b1-71f6-4ed5-8c54-xxxxx
whisk API host      https://wsk-edge.xxxx.com
whisk API version   v1
whisk namespace     _
whisk CLI version   2019-03-27T20:19:17.662+0530
whisk API build     2018-01-29T18:32:59Z
whisk API build number  latest

I am writing test cases for new output flag and also will update the documentation. Let me know your feedback.

rabbah commented 5 years ago

fantastic work 👏 @neerajmangal - thanks for these contributions.

mdeuser commented 5 years ago

this behavior is perfect @neerajmangal ! :100: :1st_place_medal: thanks!!

mdeuser commented 5 years ago

only item for discussion could be the use of std.. i was thinking off-the-cuff on that one. would default or def be better?

neerajmangal commented 5 years ago

only item for discussion could be the use of std.. i was thinking off-the-cuff on that one. would default or def be better?

Hi @mdeuser, I think we should get cut-off default|std output type itself. I mean, if --ouput|-o is provided then it will be a formatted output as raw|json|yaml otherwise it will throw an error. If --output|-o not provided then cmd will display as of today.

Let me know your suggestions.

neerajmangal commented 5 years ago

only item for discussion could be the use of std.. i was thinking off-the-cuff on that one. would default or def be better?

Hi @mdeuser, I think we should get cut-off default|std output type itself. I mean, if --ouput|-o is provided then it will be a formatted output as raw|json|yaml otherwise it will throw an error. If --output|-o not provided then cmd will display as of today.

Let me know your suggestions.

Hi @mdeuser, please let me know your thoughts on above. I should modify "std" flag further or it should be good for now?

Thanks, Neeraj

mdeuser commented 5 years ago

@neerajmangal - i'm not sure i entirely understand the following

I think we should get cut-off default|std output type itself.

imho, all supported output format types should be able to be specified via the --output|-o flag, including the default format used when none are specified.

rabbah commented 5 years ago

Is this ready to merge now?

neerajmangal commented 5 years ago

Hi @rabbah, I think we can merge it unless we want to rename "std" to "default" or "def". I can take it up first thing in the morning tomorrow if this change is required.

cc @mdeuser

rabbah commented 5 years ago

std is ok with me - will wait for Mark in case he feels strongly.

rabbah commented 5 years ago

@neerajmangal do you mind sending an email to the Apache dev list about this PR - just a short description of what you've done and a link to the PR.

mdeuser commented 5 years ago

using --output|-o raw|std works for me

rabbah commented 5 years ago

Thanks @neerajmangal for this contribution.