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

Add confirm mechanism when delete or update action #372

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ningyougang
Copy link
Contributor

@ningyougang ningyougang commented Sep 20, 2018

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.

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

@csantanapr csantanapr left a comment

Choose a reason for hiding this comment

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

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

@rabbah
Copy link
Member

rabbah commented Sep 20, 2018

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

@mdeuser
Copy link
Contributor

mdeuser commented Sep 20, 2018

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.

@rabbah
Copy link
Member

rabbah commented Sep 20, 2018

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

@csantanapr
Copy link
Member

csantanapr commented Sep 20, 2018

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.

@ningyougang
Copy link
Contributor Author

ningyougang commented Sep 21, 2018

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

@mdeuser
Copy link
Contributor

mdeuser commented Sep 21, 2018

WSK_CLI_PROMPT_ON_CHANGE and/or WSK_CLI_PROMPT do not exist in the backend

@ningyougang
Copy link
Contributor Author

ningyougang commented Sep 25, 2018

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

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

rabbah commented Sep 25, 2018

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.

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

ningyougang commented Sep 26, 2018

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

@rabbah
Copy link
Member

rabbah commented Sep 26, 2018

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

apihostSet string
apiversionSet string
namespaceSet string
promptOnChangeSet string
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already modified

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 will also need unit tests (go unit tests preferably).

@dubee
Copy link
Member

dubee commented Sep 26, 2018

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

@ningyougang
Copy link
Contributor Author

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

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

rabbah commented Sep 27, 2018

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.

@ningyougang 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
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 ningyougang force-pushed the add-confirm-when-delete-update-action branch from d84129f to 33b6054 Compare September 27, 2018 10:18
@ningyougang
Copy link
Contributor Author

ningyougang commented Sep 28, 2018

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

@ningyougang
Copy link
Contributor Author

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

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.

5 participants