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 exception for data backup #731

Merged
merged 2 commits into from Jun 17, 2019

Conversation

Projects
None yet
5 participants
@maniackcrudelis
Copy link
Contributor

commented May 29, 2019

The problem

Some users were using the setting backup_only_core to not backup data during their backup.
As this setting wasn't meant to be used this way, I didn't care when changing the way it was handled.
https://forum.yunohost.org/t/yunohost-backup-create-ko/7688

Solution

Implement a new setting do_not_backup_data to add to an app setting to avoid data backup.
This setting, as it's not part of the upgrade process, will not be removed by any script. So it could be used by an user to avoid to backup data for all backup script.

PR Status

Ready to be reviewed.

How to test

Set the setting for an app, ynh_app_setting_set $app do_not_backup_data 1

Validation

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

@YunoHost/apps

@kay0u

kay0u approved these changes May 29, 2019

Copy link
Contributor

left a comment

LGTM

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Hello,

Since a long time I thought that using the environment variable to enable some option for yunohost is not really clean. Why not adding option to the yunohost command which enable the BACKUP_CORE_ONLY environment variable.

By example we can add a flag -c or --core-only.

So if we run the command yunohost backup create --core-only --apps app1 the yunohost core get this argument and enable the BACKUP_CORE_ONLY environnement variable when the script is called.

The advantage of that is:

  • Add the possibility to add this option in the admin
  • Show cleanly this option in the help on the cli

And the best thing would be probably to do the same thing for the NO_BACKUP_UPGRADE variable.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

BACKUP_CORE_ONLY is only used by ynh_backup_before_upgrade. I see no problem in using an environment variable to do so.
There's no need to document that thing (even though it is...) since that's only an internal process for the upgrade script.

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

BACKUP_CORE_ONLY is only used by ynh_backup_before_upgrade. I see no problem in using an environment variable to do so.
There's no need to document that thing (even though it is...) since that's only an internal process for the upgrade script.

Yes but as I understand the target of this PR is for the user, not just internal. is it ?

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

But it isn't about any environment variable here !
By this PR, I add a setting which can be set for each app separately to inform the backup script to not backup data.

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Yes but my idea, was just to replace the settings by an option in the CLI which is cleaner.

Anyway do what you want, I don't mind this PR, it's just that I feel that we do many dirty things about that.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

The idea is to have a setting for each app, not for the entire backup process.
That's simply to restore what users were doing with the previous setting backup_core_only. Except that this time, this is a setting design for that purpose.

@alexAubin

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Just leaving a random idea, but we could also design something like this :

# Backup app1 (only the core), app2 and app3
yunohost backup create --apps app1::core app2 app3

# Backup app 1 (core + data)
yunohost backup create --apps app1

# One could even imagine : 
yunohost backup create --apps app1::data

That's more an "ephemeral" solution (you specify what you want to backup at command line time) but also more flexible as it allows to avoid re-editing a setting everytime you want to backup stuff differently

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

It could be an idea for temporary choosing which part of a backup you want indeed. Why not.
But again, that's not the purpose of this PR.

@zamentur
Copy link
Contributor

left a comment

LGTM

Show resolved Hide resolved data/helpers.d/backup Outdated
Update data/helpers.d/backup
Co-Authored-By: Kayou <pierre.moltess@gmail.com>
@kay0u

kay0u approved these changes Jun 14, 2019

Copy link
Contributor

left a comment

LGTM

@alexAubin alexAubin added this to the 3.6.x milestone Jun 17, 2019

@alexAubin alexAubin merged commit 3e75957 into stretch-unstable Jun 17, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the do_not_backup_data branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.