Skip to content

feat: remove compact button for older borg versions < 1.2#1727

Merged
m3nu merged 10 commits into
borgbase:masterfrom
diivi:feat/remove-compact-button-for-older-borg
Jun 23, 2023
Merged

feat: remove compact button for older borg versions < 1.2#1727
m3nu merged 10 commits into
borgbase:masterfrom
diivi:feat/remove-compact-button-for-older-borg

Conversation

@diivi
Copy link
Copy Markdown
Contributor

@diivi diivi commented Jun 5, 2023

Description

This PR removes the check for Borg version in compact.py and hides the compact button in archive_tab.py if the Borg version is less than 1.2.0. This is done to prevent errors and crashes caused by using the compact feature with an incompatible Borg version.

Related Issue

Related to:
#1723

Motivation and Context

Hides the compact button from the user instead of misleading them into thinking compact will work.

How Has This Been Tested?

Tested manually with Borg 1.2.0

Screenshots (if appropriate):

After manually changing the minimum reqd version to 1.5.0 just to test:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

@diivi diivi mentioned this pull request Jun 5, 2023
14 tasks
@diivi diivi changed the title feat: remove compact button for older borg versions feat: remove compact button for older borg versions < 1.2 Jun 5, 2023
Hofer-Julian
Hofer-Julian previously approved these changes Jun 9, 2023
Copy link
Copy Markdown
Collaborator

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

I've tested the branch locally and it works as expected.
From my side, this PR is ready to be merged.

Sidenote: I found it surprisingly hard to install an older version of borgbackup locally.
I see that #1716 used nox, but I guess that doesn't help outside of test environments?

@real-yfprojects
Copy link
Copy Markdown
Collaborator

I see that #1716 used nox, but I guess that doesn't help outside of test environments?

No, I don't think so. It uses pip under the hood afaik.

Comment thread src/vorta/borg/compact.py
Comment on lines -46 to -50
if not borg_compat.check('COMPACT_SUBCOMMAND'):
ret['ok'] = False
ret['message'] = trans_late('messages', 'This feature needs Borg 1.2.0 or higher.')
return ret

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of removing this you could raise an Exception 🤷

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right. In case the code is somehow run with the wrong version. But how would that work if the same check already runs elsewhere? Who knows..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, even I thought it might be redundant but ig good to have it.

Comment thread src/vorta/views/archive_tab.py
Comment thread src/vorta/views/archive_tab.py
Comment thread src/vorta/views/archive_tab.py
@diivi diivi requested a review from real-yfprojects June 16, 2023 18:55
m3nu
m3nu previously approved these changes Jun 23, 2023
@m3nu m3nu self-assigned this Jun 23, 2023
@m3nu m3nu self-requested a review June 23, 2023 12:57
@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented Jun 23, 2023

Tested locally and the button state becomes enabled again after changing the profile.

@m3nu m3nu dismissed their stale review June 23, 2023 13:00

found bug during local testing

@diivi
Copy link
Copy Markdown
Contributor Author

diivi commented Jun 23, 2023

Fixed.
Peek 2023-06-24 00-32

Copy link
Copy Markdown
Contributor

@m3nu m3nu left a comment

Choose a reason for hiding this comment

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

Tested locally and was working.

@m3nu m3nu merged commit f76195e into borgbase:master Jun 23, 2023
@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented Jun 23, 2023

Another one in the bag. 👍

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