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

Fix counter-intuitive backup API #490

Merged
merged 2 commits into from
Jun 15, 2018
Merged

Conversation

alexAubin
Copy link
Member

The problem

c.f. YunoHost/issues#1102

Currently there are some counter-intuitive behaviors with backup create and backup restore.

Typically, yunohost backup create --apps wordpress will backup wordpress and the whole system. You need to explicitly add --ignore-system at the end. Similarly, yunohost backup create --system ldap will backup the ldap base and all the apps. This kind of stuff also apply for the restore, so yunohost backup restore --apps wordpress will restore wordpress and all the system ...

This would be nice to have this fix for stretch as we can expect several people to be willing to use the backup/restore instead of the straight migration.

Solution

  • (Remove old deprecated --hooks options)
  • Remove --ignore-system and --ignore-apps
  • Only backup/restore what is explicitly given in argument (or everything if neither --apps or --system are given)

To clarify, here's a table summarizing the behavior for a few example :

Commnand Behavior
yunohost backup create Create a backup of everything (apps and system)
yunohost backup create --apps Create a backup of all apps (no system)
yunohost backup create --apps wordpress Create a backup of wordpress only
yunohost backup create --apps wordpress --system Create a backup of wordpress only + all the system
yunohost backup create --apps wordpress --system ldap Create a backup of wordpress only + ldap base

(Same kind of behavior for restore)

PR Status

To be tested

How to test

Pull the branch and maybe add some debug info + a return to see what it would do without actually doing it ...

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

Copy link
Member

@Psycojoker Psycojoker left a comment

Choose a reason for hiding this comment

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

Principle approval.

(while I'm not sure it's the good time to do that)

Copy link
Member

@zamentur zamentur left a comment

Choose a reason for hiding this comment

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

LGTM, untested. I will test it with my new borg_ynh apps

@alexAubin alexAubin merged commit 8268d6e into stretch-unstable Jun 15, 2018
@alexAubin alexAubin deleted the fix-backup-api branch June 15, 2018 17:30
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.

3 participants