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

Auto supply a dummy API GW token #1068

Merged
merged 2 commits into from
Sep 11, 2019

Conversation

jiangpengcheng
Copy link
Contributor

Since the APIGW_ACCESS_TOKEN is an optinal parameter, and the action apimgmt/createApi do require this parameter no matter what the value is, I think it's better to provide a fake token just like the openwhisk-cli did, see api.go:

related to: #1037

Copy link
Contributor

@mrutkows mrutkows left a comment

Choose a reason for hiding this comment

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

Please use constant and provide feedback to user that a default value will be used and display that value (i.e., "DUMMY TOKEN").

wski18n.ID_MSG_CONFIG_MISSING_APIGW_ACCESS_TOKEN)
warningString := wski18n.T(wski18n.ID_MSG_CONFIG_MISSING_APIGW_ACCESS_TOKEN)
wskprint.PrintOpenWhiskWarning(warningString)
client.ApigwAccessToken = "DUMMY TOKEN"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make the string "DUMMY TOKEN" a package constant and also "wskprint" that we are using a default string with the value "DUMMY TOKEN" so it is clear utility is providing a default behavior (due to lack of explicit value)?

@mrutkows
Copy link
Contributor

@jiangpengcheng Thanks for finding and making this correction. If you make these simple changes requested in the review comments, I plan to merge.

@jiangpengcheng
Copy link
Contributor Author

ok, I use a constant for the dummy token, and print a more clear message for users

Copy link
Contributor

@mrutkows mrutkows left a comment

Choose a reason for hiding this comment

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

Excellent use of the i18n resource files.

@mrutkows mrutkows merged commit 01cfe67 into apache:master Sep 11, 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.

2 participants