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

Back up to multiple repos from one profile #942 (duplicate) #1005

Closed
wants to merge 31 commits into from

Conversation

bastiencyr
Copy link
Collaborator

@bastiencyr bastiencyr commented Jun 3, 2021

Hello,
It's the same pull request without noising commits.
Sum up : it is still missing a solution for archive.

  • database (create table, migration, methods...)
  • checkBoxes for repos
  • support profile import/export
  • comboBox in repoArchive
  • comboBox for shell command
  • real create backup cmd support (multiple task queues is a better idea)

@m3nu
Copy link
Contributor

m3nu commented Jun 3, 2021

Thanks! You could also squash and force-push to clean up commits. Same result.

@bastiencyr
Copy link
Collaborator Author

bastiencyr commented Jun 5, 2021

Last commit on master has caused a lot of conflicts. Tests don't pass anymore. I will need time to fix that :(
(btw, very nice new feature <3 )

@m3nu
Copy link
Contributor

m3nu commented Jun 5, 2021

We have to thank @phihos for that.

The secret bootstrapping feature is explained here: https://vorta.borgbase.com/usage/settings/

@bastiencyr
Copy link
Collaborator Author

Hello @m3nu ,
I go forward little by little. I manage to understand why activated signal made the tests failed. Import/export profile are supported but I think I need some advices from @phihos .
Sorry, sometimes I push while code tests dont pass. it's because on Linux Mint, 4 tests never pass even on master branch (test_profile_add, test_profile_edit, test_repo_add_failures, test_password_autofill).

@phihos
Copy link
Contributor

phihos commented Jun 14, 2021

Hi @bastiencyr, I checked out your branch and the tests seem to run fine. I have a hard time to understand your intentions just by looking at the diffs. There are also a lot of import reorderings and whitespace removals/additions that make it hard for me to spot the meaningful semantic changes. Just some unsolicited advice from me: Try to put code style cleanup of code you do not need to change into an additional PR to make it easier for reviewers.
Another issue I encountered was that your changes are not downward compatible. I can not import schema v17 exports anymore. Instead of changing the existing test data I suggest to have two valid.json files: A valid_v17.json and a valid_v18.json. You could use something like pytest parametrize to run the same test with both exports and change the code until both exports can be imported.

With that out of the way, how can I help you? 😃
If you could provide a failing test showing what you like to achieve would help me to understand how to help.

@bastiencyr
Copy link
Collaborator Author

bastiencyr commented Jun 15, 2021

Hi @bastiencyr, I checked out your branch and the tests seem to run fine. I have a hard time to understand your intentions just by looking at the diffs. There are also a lot of import reorderings and whitespace removals/additions that make it hard for me to spot the meaningful semantic changes. Just some unsolicited advice from me: Try to put code style cleanup of code you do not need to change into an additional PR to make it easier for reviewers.
Another issue I encountered was that your changes are not downward compatible. I can not import schema v17 exports anymore. Instead of changing the existing test data I suggest to have two valid.json files: A valid_v17.json and a valid_v18.json. You could use something like pytest parametrize to run the same test with both exports and change the code until both exports can be imported.

Yes it's not compatible with current version. It will be difficult to maintain compatibility for import profile. Do I have to ensure compatibility ? If I do, there are 2 scenarios :

  1. Someone with json v17 try to import in v18
  2. Someone with json v18 try to import in v17 -> there is currently no error but actually, it is not supported. In this case, I think that the best way is to ask to the user to upgrade his version.

I note for style clean-up, I will do better next time :(

With that out of the way, how can I help you? smiley
If you could provide a failing test showing what you like to achieve would help me to understand how to help.

It's about property decorator in profile_export.py. I dont think I can use them repo_password for example since there is a variable number of password (for each repo). So, for the moment I return the entire self._profile_dict['BackupProfileMixin'] and directly edit it in import_window.py. Is it ok ?

Thank you for helping :)

@m3nu
Copy link
Contributor

m3nu commented Jun 15, 2021

When fields change, can't we just ignore the extra ones and take default values for new ones?

Apart from that there could just be an error when the version doesn't match.

@bastiencyr
Copy link
Collaborator Author

Apart from that there could just be an error when the version doesn't match.

It's a good solution.

When fields change, can't we just ignore the extra ones and take default values for new ones?

What field are you talking about?

@bastiencyr
Copy link
Collaborator Author

bastiencyr commented Jul 8, 2021

Hello @m3nu ,
I think this feature is stable. I have two problems :

  • I cant enable profile.repo.create_backup_cmd because create_backup_cmd is added at level repository. So a solution is to add a comboBox in which user select a repo to add this cmd. But the ui will become too much complex I think (there is already a combobox to select repos in archive tab). A better solution is to create a tab for settings repos only... The unlink repository can be moved in this tab for example.
  • I cant pass the test create lock because create lock is never used in the code

EDIT, I think that create_backup_cmd should be added at profile level.

Otherwise, it works well

@m3nu
Copy link
Contributor

m3nu commented Jul 8, 2021

Good stuff.👍 I'll take a look at the tests to see what can be done here.

@m3nu
Copy link
Contributor

m3nu commented Jul 12, 2021

I'm traveling this week and will need a bit longer to get started on this. But it's highest on my list after that, since it's a great feature.

@bastiencyr bastiencyr mentioned this pull request Jul 27, 2021
7 tasks
@m3nu
Copy link
Contributor

m3nu commented Aug 26, 2021

Sorry for the delay. I'll try to work on this today. Want to do a minor release first because my Apple Developer Membership expires soon.

@m3nu
Copy link
Contributor

m3nu commented Aug 26, 2021

I'm releasing v0.7.8 now. The next version, 0.8 will include larger changes, like this PR.

@bastiencyr
Copy link
Collaborator Author

bastiencyr commented Sep 5, 2021

I'm releasing v0.7.8 now. The next version, 0.8 will include larger changes, like this PR.

No problem, this PR is not my priority anymore. I am focus on multiple queues implementation #1045 . I will be back there after.

@m3nu
Copy link
Contributor

m3nu commented Sep 5, 2021

Good. If you keep your PRs small and targeted, it's quicker to merge it, because the likelihood of new bugs is lower. I know that's not always possible..

@bastiencyr bastiencyr added this to the v0.8 (Next major release) milestone Oct 4, 2021
@bastiencyr bastiencyr self-assigned this Oct 6, 2021
@bastiencyr bastiencyr force-pushed the repos branch 2 times, most recently from ed0aaa0 to e318709 Compare October 7, 2021 22:43
@bastiencyr
Copy link
Collaborator Author

Restarting the discussion, I was able to properly implement this feature without recursive function thanks to the jobsmanager :) I pass test create lock !
But notice that profile.repo.create_backup_cmd (add argument to backup command) feature is not implemented for the same reason given above :(

@bastiencyr
Copy link
Collaborator Author

bastiencyr commented Nov 4, 2021

Hello @m3nu, I really would like to finish this PR before opening another one : rebasing is a little bit exhausting. But I cant continue without knowing what we do with profile.repo.create_backup_cmd .

@bastiencyr bastiencyr removed their assignment Jan 21, 2022
@git70
Copy link

git70 commented Feb 8, 2022

  1. Do I understand that this feature will enable such?
  2. If so, I have an idea for its additional extension:
    Let the user choose between:
    a) perform a sequentially backup tasks
    b) perform backup simultaneously to various repositories
    I know that Borg can not, but you can run several Borg processes simultaneously. @ThomasWaldmann mentioned it here

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.

4 participants