Add confirm mechanism when delete or update action by ningyougang · Pull Request #372 · apache/openwhisk-cli · GitHub

I writed a issue here: #371

As more and more production system uses openwhisk,
Users will need some feature to protect their action to be
deleted or updated by mistake.

I disagree with this change as would force a current user to always use this new flag when deleting entity.

One example of a more popular CLI is Kubernetes and it doesn’t require a flag to delete
https://kubernetes.io/docs/reference/generated/kubectl/kubectl-commands#delete

Top level program calling CLI can do this check
You can extend/fork the code and create your own customized of the CLI (IBM already does with ibm wsk ...)

What about adding a—safe mode which will prevent delete without a confirmation?

this appears to be a breaking change as it is currently implemented. if so, i'm not in favor of this change as implemented.

one possible implementation is to use an env variable, say WSK_CLI_SAFE_MODE or WSK_CLI_ACTION_MGMT_SAFE_MODE or..., that when set will cause the cli to always prompt for confirmation of action deletes/updates. when env is not present, the current behavior remains. no need for additional command line flag.

I like the suggestions. Could also store it in whisk properties.

yep I like the using environment variable.
But I'm not in love with the wording SAFE.
How about something along WSK_CLI_PROMPT_ON_CHANGE or WSK_CLI_PROMPT=true|false (default) to false.
Also this PR only touches on actions.
I think it should be done for all changes (ie. wsk update | delete) I guess the user wants to be prompted if by mistake it updates or deletes a rule.

@csantanapr , thanks for your reply.
if used WSK_CLI_PROMPT_ON_CHANGE or WSK_CLI_PROMPT=true|false
Does this env WSK_CLI_PROMPT_ON_CHANGE existed in openwhisk server side, exactly controller side(not in wsk client side)?

WSK_CLI_PROMPT_ON_CHANGE and/or WSK_CLI_PROMPT do not exist in the backend

@mdeuser
Regarding WSK_CLI_PROMPT_ON_CHANGE and/or WSK_CLI_PROMPT do not exist in the backend

so you mean WSK_CLI_PROMPT_ON_CHANGE and/or WSK_CLI_PROMPT exists in wsk client side?
if exist in wsk client side, for wsk client users, they may not know above environment env.
How to notify wsk client users to know above environment env?

A property saved in wskprops would address the environment issue since switching files will then configure the cli as desired. We have no precedent for an env file that changes the cli behavior outside of WSK_CONFIG_FILE so a stand-alone property to me for this behavior seems like the wrong direction.

@rabbah ,already modified.

The patch implement the protect feature at CLI layer.

How about add the protect mechanism at the backed server layer also?
because users may deploy their action using API (not wsk CLI)

But i am afraid that if added in backed server layer, may lead to client side do huge breakpoint.
How do you think?

On the backend I think the model is finer grained entitlement than we have today, with unix style permissions.

can this be a boolean flag? i.e. the present of --confirm (typical practice in other clis use --force) indicates the desire to override the confirmation.

Already modified

This will also need unit tests (go unit tests preferably).

I think a prompt would be better than an additional flag.

@rabbah , so we have no need to add the protect mechanism for backend layer, right?

If by prompt you mean something interactive which requires input on the console then this will make it very inconvenient to actually update a namespace when it is so intended.

As more and more production system uses openwhisk, Users will need some feature to protect their action to be deleted or updated by mistake.

@rabbah ,already added test cases.

Regarding add protection feature to backend layer, i also think it these days.
First, we should consider the original created action,
if we add protection feature in backend layer ,we should promise have no influences to the original created action.

So one option is, add promptOnChange fileld to action, default value is false,
When create action, we can assign the promptOnChange to true.
if that, when call delete action API, should add force request parameter to delete it.

If we add promptOnChange field to backend's action, so have no need to add promptOnChange to WSK's wskprops file.
How do you think?

Please don't merge this patch
May be we can solve this problem in backend layer:
apache/openwhisk#4058

ningyougang force-pushed the add-confirm-when-delete-update-action branch from ba7141a to 62cab2f Compare September 20, 2018 08:34

csantanapr requested changes Sep 20, 2018

View reviewed changes

ningyougang force-pushed the add-confirm-when-delete-update-action branch from 62cab2f to 8294483 Compare September 25, 2018 10:01

ningyougang force-pushed the add-confirm-when-delete-update-action branch from 8294483 to 247866e Compare September 26, 2018 05:51

mdeuser reviewed Sep 26, 2018

View reviewed changes

rabbah reviewed Sep 26, 2018

View reviewed changes

ningyougang force-pushed the add-confirm-when-delete-update-action branch 4 times, most recently from f6017df to b63643e Compare September 27, 2018 07:51

ningyougang force-pushed the add-confirm-when-delete-update-action branch 3 times, most recently from 7a1a4c5 to d84129f Compare September 27, 2018 10:17

Add prompt on change when delete or update action

33b6054

As more and more production system uses openwhisk, Users will need some feature to protect their action to be deleted or updated by mistake.

ningyougang force-pushed the add-confirm-when-delete-update-action branch from d84129f to 33b6054 Compare September 27, 2018 10:18

ningyougang mentioned this pull request Oct 9, 2018

Add protect feature to avoid update or delete actions by mistake apache/openwhisk#4058

Closed

21 tasks

+              		apihostSet        string
+              		apiversionSet     string
+              		namespaceSet      string
+              		promptOnChangeSet string

Add confirm mechanism when delete or update action by ningyougang · Pull Request #372 · apache/openwhisk-cli · GitHub

Uh oh!

Add confirm mechanism when delete or update action - #372

Add confirm mechanism when delete or update action#372ningyougang wants to merge 1 commit intoapache:masterapache/openwhisk-cli:masterfrom ningyougang:add-confirm-when-delete-update-actionningyougang/incubator-openwhisk-cli:add-confirm-when-delete-update-actionCopy head branch name to clipboard

Conversation

ningyougang commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csantanapr left a comment

Choose a reason for hiding this comment

Uh oh!

rabbah commented Sep 20, 2018

Uh oh!

mdeuser commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rabbah commented Sep 20, 2018

Uh oh!

csantanapr commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ningyougang commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdeuser commented Sep 21, 2018

Uh oh!

ningyougang commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rabbah commented Sep 25, 2018

Uh oh!

ningyougang commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rabbah commented Sep 26, 2018

Uh oh!

mdeuser Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

ningyougang Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

rabbah left a comment

Choose a reason for hiding this comment

Uh oh!

dubee commented Sep 26, 2018

Uh oh!

ningyougang commented Sep 27, 2018

Uh oh!

rabbah commented Sep 27, 2018

Uh oh!

ningyougang commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ningyougang commented Oct 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Add confirm mechanism when delete or update action#372
ningyougang wants to merge 1 commit into
apache:masterfrom
ningyougang:add-confirm-when-delete-update-action

ningyougang commented Sep 20, 2018 •
edited
Loading

mdeuser commented Sep 20, 2018 •
edited
Loading

csantanapr commented Sep 20, 2018 •
edited
Loading

ningyougang commented Sep 21, 2018 •
edited
Loading

ningyougang commented Sep 25, 2018 •
edited
Loading

ningyougang commented Sep 26, 2018 •
edited
Loading

ningyougang commented Sep 28, 2018 •
edited
Loading