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

[enh] allow to have arguments in manifest for the remove script #751

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

Conversation

@Psycojoker
Copy link
Member

@Psycojoker Psycojoker commented Jul 9, 2019

The problem

It's not possible to ask questions to user during the removal of an app, this cause problem for undefined behavior where we aren't sure what is the best course of action. The most common case is: should we keep nextcloud data?

Solution

Allow to ask questions to user.

A serie of personal suggestions:

  • I think that all apps should provide default value for all remove arguments
  • the app groupe should discuss the general question of "should we keep app data?" default answer to uniformize this behavior amongs all apps, I'm expecting this to be the number of question that is going to pop out

PR Status

Tested on my installation, works as excepted.

THE ADMIN INTERFACE PART IS NOT DONE. Mostly because bower fails without any reason for me x(

How to test

You simply need to add a "remove" section in "arguments" exactely the same one than "install" in a random app.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@Psycojoker Psycojoker force-pushed the arguments_on_remove branch from dee271d to d647b1b Jul 9, 2019
Copy link
Member

@alexAubin alexAubin left a comment

Not tested but LGTM

Loading

if "remove" in manifest.get("arguments", {}) and manifest["arguments"]["remove"]:
args_odict = _parse_args_from_manifest(manifest, 'remove', args=args_dict)
args_list = [value[0] for value in args_odict.values()]
args_list.append(app)
Copy link
Member

@alexAubin alexAubin Aug 5, 2019

Choose a reason for hiding this comment

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

Uh I have no idea what I'm talking about but shouldn't app be in first position so that it corresponds to $1 ? (Well I guess anyway all recents apps likely to require argument for remove don't rely on $1, $2, ... anymore)

Loading

@@ -993,16 +994,41 @@ def app_remove(operation_logger, app):
os.system('chown -R admin: /tmp/yunohost_remove')
os.system('chmod -R u+rX /tmp/yunohost_remove')

args_list = [app]
with open(os.path.join(APPS_SETTING_PATH, app, 'manifest.json')) as json_manifest:
manifest = json.load(json_manifest)
Copy link
Member

@alexAubin alexAubin Aug 5, 2019

Choose a reason for hiding this comment

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

To be kept in mind that this should be replaced by the get_manifest_for_app thing from the other toml manifest PR

Loading

@alexAubin alexAubin force-pushed the stretch-unstable branch from 58ead9e to 40f48ff Oct 29, 2019
@zamentur zamentur added this to the 4.3.x milestone Jan 3, 2021
@zamentur zamentur changed the base branch from stretch-unstable to dev Jan 3, 2021
@zamentur zamentur removed this from the 4.x milestone Jan 4, 2021
@zamentur zamentur added this to 4.x in Pending Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants