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

Added --keep-all, as an alias for '--keep-last <inf>', as an option… #7462

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Michael-Girma
Copy link
Contributor

@Michael-Girma Michael-Girma commented Mar 22, 2023

… for borg-prune. Fixes #6656

  • Added a subparser argument that stores a large value for --keep-last when pruning.

@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2023

Codecov Report

Merging #7462 (0301451) into master (1e1c922) will increase coverage by 0.00%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #7462   +/-   ##
=======================================
  Coverage   83.87%   83.87%           
=======================================
  Files          67       67           
  Lines       11817    11818    +1     
  Branches     2155     2155           
=======================================
+ Hits         9911     9912    +1     
  Misses       1332     1332           
  Partials      574      574           
Impacted Files Coverage Δ
src/borg/archiver/prune_cmd.py 87.14% <100.00%> (+0.09%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ThomasWaldmann
Copy link
Member

Hehe, looks good. Considering the special infinity value, I guess a test for this would be good (just test the code does not crash and keeps all archives present, except old checkpoint archives).

@jdchristensen
Copy link
Contributor

What happens if someone does --keep-all --keep-secondly=10? Does the 10 overwrite the infinite value? This might be surprising to the user since the options seem independent. (This is already an issue with --keep-last 100 --keep-secondly 10 as well, I believe.) Maybe --keep-last should be implemented separately from --keep-secondly? (I'd also be inclined to remove --keep-secondly in borg2.) (I'm not worried about the interaction between --keep-all and --keep-last, since --keep-all is documented to be equivalent to --keep-last <inf>.)

The above conflicts might happen because someone has a script that already has various --keep options, and adds --keep-all at the front, thinking that this would cause everything to be kept.

@Michael-Girma
Copy link
Contributor Author

@jdchristensen, interesting insight. I'll take a look at the interactions between those flags. @ThomasWaldmann I did test that eye-catching infinite value locally (didn't test deeply, just created a few archives to see that it didn't crash). I can write up a unit test to see how well it works with keeping archives and removing checkpoints. I would love to hear your thoughts on @jdchristensen's remarks though.

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Mar 22, 2023 via email

@ThomasWaldmann
Copy link
Member

If we have different argparse options, we could also use this:

https://docs.python.org/3/library/argparse.html#mutual-exclusion

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Apr 8, 2023

I resolved the merge conflict and also had another look:

  • Highlander is not needed for --keep-all - it can be given multiple times without causing an issue (because it is always using the same infinity value)
  • but we should have the mutual exclusion for --keep-last, --keep-secondly, --keep-all.

@Michael-Girma Can you try to implement that and add a test?

@ThomasWaldmann
Copy link
Member

@Michael-Girma did you see my last comment, can you fix it?

@Michael-Girma
Copy link
Contributor Author

@ThomasWaldmann just seeing this. Will take a look tonight.

@borgbackup borgbackup deleted a comment from Michael-Girma May 16, 2023
@ThomasWaldmann
Copy link
Member

ping.

@ThomasWaldmann
Copy link
Member

ping

1 similar comment
@ThomasWaldmann
Copy link
Member

ping

@ThomasWaldmann
Copy link
Member

@Michael-Girma do you need help finishing this?

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.

Add borg prune --keep-all
4 participants