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

Remove misleading web-export key #1092

Merged
merged 3 commits into from
Apr 30, 2020
Merged

Conversation

style95
Copy link
Member

@style95 style95 commented Mar 24, 2020

web-export(key) is being interchangeably used with web in wskdeploy.
On the other hand, it is differently used in wsk cli.

For example, if the web flag is enabled OW will make the action as a web action and enable the final flag. To avoid the use of protected parameters, users can configure two annotations, web-export(true) and final(false) flag.

But with wskdeploy, web-export is also being used as a top key of actions and it enables the final flag all the time.

Even if users still can control this with annotations, I think it is misleading to have the same option in two different places with two different meanings.

So I removed the top-level one.

@mrutkows
Copy link
Contributor

mrutkows commented Mar 25, 2020

@style95 There is historical context for this web-export being added at a higher level than annotations... that is, we had intended that wskdeploy manifests would not allow/expose annotations period (at all). However, we were forced by some use cases to add support for it.

My concern is that the code SHOULD still permit the web-export key outside annotations (that is still work and not break anyone using it for 3 years), but deprecate it. Then update all examples use cases to use the annotations, but preserve at least one functional test to make sure that, although deprecated, it still works.

@style95
Copy link
Member Author

style95 commented Mar 26, 2020

@mrutkows Thank you for the good comment.
I would update it accordingly.

@style95
Copy link
Member Author

style95 commented Apr 13, 2020

@mrutkows
I initially thought of explicitly specifying the deprecation of the top-level web-export field.
But since both top-level web-export and annotations.web-export can coexist, I think it would be enough to just change the top-level one to the annotations' one in all occurrences.
Newbies will start using the annotation.web-export as the top-level one is no more exposed.

Do you think we have to explicitly specify the deprecation?

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.

@style95 Thanks for changing all the tests and docs to reflect our preferred use of web export flag under annotations.

@mrutkows mrutkows merged commit 5073943 into apache:master Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants