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

Support multiple repositories (values separated by ;) #149

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

Conversation

fflorent
Copy link
Contributor

Problem

Fixes #146

When backuping with multiple repositories, you'd have to install twice the borg app.
If I am not wrong, this causes to copy (link) apps data as many times as you have repositories, which can take time depending on the apps being backed up.

Solution

Instead, this patch proposes to install a single client for multiple repositories.

Note: Probably this PR would need #148, if we'd prefer separating values by newlines instead of semi-colons.

PR Status

  • Code finished and ready to be reviewed/tested
  • The fix/enhancement were manually tested (if applicable)

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

Copy link
Contributor

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

In fact, only db data are copied twice in this case. YunoHost use mount --bind to avoid to copy directory (that's why you dont' need to much space to backup your system).

With recent version of borg (and change in yunohost behavior), we could even imagine to pipe db export into borg command to avoid intermediary copy...)

In more, having 2 borg setup allows to trigger backup on 2 distinct date, which can be helpful.

@@ -5,13 +5,18 @@ app="${0#"./05-"}"
app="${app%"_app"}"

BORG_PASSPHRASE="$(yunohost app setting $app passphrase)"
repo="$(yunohost app setting $app repository)" #$4
repos="$(yunohost app setting $app repository)" #$4
if ssh-keygen -F "__SERVER__" >/dev/null ; then
Copy link
Contributor

@zamentur zamentur Apr 14, 2024

Choose a reason for hiding this comment

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

if there are several repos, i guess we should adapt this part to. __SERVER__ is extracted from repository var if i remember well. And i think we should improve this part by deducing server from $repository (instead of hradcoding it during install script...)

;;
backup)
do_backup "$work_dir" "$name" "$repo" "$size" "$description"
for repo in "${array_repos[@]}"; do
do_backup "$work_dir" "$name" "$(sanitize_repo "$repo")" "$size" "$description"
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind if the first borg command to create the repo failed, the second won't be runned... (so it's not totally the same behavior than install twice the app)

@zamentur
Copy link
Contributor

This part should be adapted to : https://github.com/YunoHost-Apps/borg_ynh/blob/master/scripts/config#L50

@zamentur
Copy link
Contributor

And also the manifest.toml (testing branch) and the config_panel.toml

@fflorent
Copy link
Contributor Author

@zamentur Right, thanks for the explanation, so knowing that, do you think it could be still a good idea?
I don't mind closing this PR if there is not much benefits.

@zamentur
Copy link
Contributor

I guess it's still a good idea for apps which have a big db (for example synapse).

@alexAubin alexAubin deleted the branch YunoHost-Apps:testing August 5, 2024 16:37
@alexAubin alexAubin closed this Aug 5, 2024
@alexAubin alexAubin reopened this Aug 5, 2024
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.

3 participants