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

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

Merged
merged 8 commits into from
Apr 17, 2019

Conversation

neerajmangal
Copy link
Contributor

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
Copy link
Contributor Author

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

@mdeuser
Copy link
Contributor

mdeuser commented Mar 26, 2019

@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)

Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

This is awesome! Thank you for implementing this.

@neerajmangal
Copy link
Contributor Author

neerajmangal commented Mar 26, 2019

@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
Copy link
Contributor

mdeuser commented Mar 26, 2019

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
Copy link
Contributor Author

neerajmangal commented Mar 27, 2019

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
Copy link
Member

rabbah commented Mar 27, 2019

fantastic work 👏 @neerajmangal - thanks for these contributions.

Copy link
Member

@dubee dubee 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!

@mdeuser
Copy link
Contributor

mdeuser commented Mar 27, 2019

this behavior is perfect @neerajmangal ! 💯 🥇 thanks!!

@mdeuser
Copy link
Contributor

mdeuser commented Mar 27, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

mdeuser commented Apr 5, 2019

@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
Copy link
Member

rabbah commented Apr 10, 2019

Is this ready to merge now?

@neerajmangal
Copy link
Contributor Author

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
Copy link
Member

rabbah commented Apr 10, 2019

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

@rabbah
Copy link
Member

rabbah commented Apr 10, 2019

@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
Copy link
Contributor

mdeuser commented Apr 10, 2019

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

@rabbah
Copy link
Member

rabbah commented Apr 17, 2019

Thanks @neerajmangal for this contribution.

@rabbah rabbah merged commit 23de5a3 into apache:master Apr 17, 2019
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.

4 participants