Archives filters by funkyfuture · Pull Request #1554 · borgbackup/borg

funkyfuture · 2016-08-30T20:16:59Z

follow-up of #1369

this adds the arguments --first, --last and --sort-by, and groups them with --prefix as archives filters that can be applied on repository targets.

usable with check, delete, info and list.

use-case in a script:

while used_space_exceeds_pcent; do
    log 'Used space exceeds ${MAX_DISK_USAGE}%, deleting oldest backup archive.'
    borg delete --first 1 --force $BORG_REPOSITORY & wait
done

funkyfuture · 2016-08-30T20:21:38Z

strange, locally 42cd980 appears as second of these commits.

codecov-io · 2016-08-30T21:22:04Z

Current coverage is 84.68% (diff: 84.74%)

Merging #1554 into master will decrease coverage by 0.07%

@@             master      #1554   diff @@
==========================================
  Files            20         20          
  Lines          6481       6548    +67   
  Methods           0          0          
  Messages          0          0          
  Branches       1093       1112    +19   
==========================================
+ Hits           5493       5545    +52   
- Misses          727        735     +8   
- Partials        261        268     +7

Powered by Codecov. Last update f276e30...089995e

ThomasWaldmann · 2016-08-30T22:13:18Z

+        group.add_argument('--first', dest='first', metavar='N', default=0, type=int,
+                           help='delete N first archives')
+        group.add_argument('--last', dest='last', metavar='N', default=0, type=int,
+                           help='delete N last archives')


it always says "delete", no matter to where you add these filter args.
so, how about "consider N first/last archives".
please create a fixup commit just for this.

funkyfuture · 2016-08-31T20:57:00Z

ai'ght, thanks for the feedback. i changed the commits where the solution was clear to me.

ThomasWaldmann · 2016-08-31T21:27:18Z

+        group.add_argument('--first', dest='first', metavar='N', default=0, type=int,
+                           help='select first N archives')
+        group.add_argument('--last', dest='last', metavar='N', default=0, type=int,
+                           help='delete last N archives')


you call this method from list, delete, check, ... right?
so the help texts are still wrong.
i suggested the wording "consider first/last N archives" in a previous review.

oops. i made it a little more explicit now: consider last N archives after other filter args were applied. that okay?

funkyfuture · 2016-09-01T15:54:56Z

okay, i did a fix commit concerning the remaining issues. to rebase it is kind of a pita.

ThomasWaldmann · 2016-09-03T17:36:48Z

any idea how to review d61c778 ?

funkyfuture · 2016-09-04T12:40:49Z

maybe with two screens, reading both revisions side-by-side rather than with a diff view

ThomasWaldmann · 2016-09-04T13:36:54Z

Sorry, I can't review d61c778 .

It somehow looks better than the old code, but I can not see whether it still does the same thing and works correctly (except what is covered by the still succeeding unit tests). At the same time, this is critical code with the capability to either not repair your repo or make stuff even worse, if things go wrong.

So, anybody else wants to review this?

Or, remove it from this PR to get the rest adopted more quickly?

In general, I would advise doing less complex/big changesets and especially hold back from refactoring / cleaning up and introducing new features within the same changeset (or even PR) - it just messes up the limited diff view on github and makes things hard to review.

funkyfuture · 2016-09-21T21:39:22Z

since you changed the ArchiveChecker and i was really not keen to rebase onto that, i removed the commit in question.

concerning the review, i have no other idea than (more?!) tests and reading.

enkore · 2016-09-30T15:13:30Z

Can you rebase this on current master? Thanks!

ThomasWaldmann · 2016-09-30T15:17:25Z

+
+        group = filters_group.add_mutually_exclusive_group()
+        group.add_argument('--first', dest='first', metavar='N', default=0, type=int,
+                           help='consider first N archives after other filter args were applied')


nitpick: "after other filters were applied"

ThomasWaldmann · 2016-09-30T15:17:39Z

+        group.add_argument('--first', dest='first', metavar='N', default=0, type=int,
+                           help='consider first N archives after other filter args were applied')
+        group.add_argument('--last', dest='last', metavar='N', default=0, type=int,
+                           help='consider last N archives after other filter args were applied')


ThomasWaldmann · 2016-09-30T15:25:43Z

+    def list(self, sort_by=None, reverse=False, prefix=''):
+        """ Inexpensive Archive.list_archives replacement if we just need .name, .id, .ts
+            Returns list of borg.helpers.ArchiveInfo instances
+        """


docstring style:
"""
like
this
"""
or
"""like this"""

ThomasWaldmann · 2016-09-30T15:36:58Z

+        if not archives:
+            logger.critical('There are no archives.')
+            self.exit_code = self.exit_code or EXIT_WARNING
+            return []


please remove the complete if-block, it is rather harmful than doing anything good.
if archives is empty here, the rest of the code here will still work normally and return a empty list at the end, no special casing needed.

ThomasWaldmann · 2016-09-30T15:45:41Z

@funkyfuture if we can finish this quickly, it can go into 1.1.0b2 still, release is tomorrow.

funkyfuture · 2016-09-30T15:47:10Z

i'll try to get it done this night.

ThomasWaldmann · 2016-09-30T15:47:44Z

Cool, thanks.

These are: --sort-by, --first and --last Includes a method to obtain a list of archive infos filtered by these Archives.list: - ensure reverse is always applied - always return a list

- adds prefix argument to helpers.Archives.list - also renames function PrefixSpec to prefix_spec

funkyfuture · 2016-09-30T17:58:17Z

can you give me a hint what is suddenly causing this:

user@host:~/python/borg (archives_filters) $ borg init --encryption=none /tmp/test
self test test_iteritems (borg.testsuite.hashindex.HashIndexTestCase) FAILED:
Traceback (most recent call last):
  File "/home/user/python/borg/src/borg/testsuite/hashindex.py", line 91, in test_iteritems
    self.assert_raises(StopIteration, next, iterator)
  File "/usr/lib/python3.5/site-packages/_pytest/python.py", line 1202, in raises
    pytest.fail(message)
  File "/usr/lib/python3.5/site-packages/_pytest/runner.py", line 537, in fail
    raise Failed(msg=msg, pytrace=pytrace)
Failed: DID NOT RAISE <class 'StopIteration'>

self test failed
This is a bug either in Borg or in the package / distribution you use.

enkore · 2016-09-30T18:05:23Z

Recompile the extensions (python setup.py develop or pip install -e .); it seems we forgot to bump the API_VERSION of hashindex there (#1659).

funkyfuture · 2016-09-30T18:23:49Z

i incorporated the demanded changes, but did not squash these as the effort and risk to break things are too high.

ThomasWaldmann · 2016-09-30T18:28:07Z

./src/borg/testsuite/archiver.py:1830:5: E303 too many blank lines (2)

ThomasWaldmann · 2016-09-30T18:29:24Z

lgtm modulo test results.

funkyfuture · 2016-09-30T18:29:46Z

got it.

ThomasWaldmann

Thanks! Code definitely more clean now.

Please note that there are some followup tickets with bounties on the issue tracker.

ThomasWaldmann · 2016-09-30T19:14:36Z

2 related issues with bounties:
#1663
#1542

funkyfuture mentioned this pull request Aug 30, 2016

Extends delete command with --oldest & --latest options #1369

Closed

ThomasWaldmann reviewed Aug 30, 2016
View reviewed changes

ThomasWaldmann reviewed Aug 31, 2016
View reviewed changes

ThomasWaldmann reviewed Sep 30, 2016

View reviewed changes

ThomasWaldmann mentioned this pull request Sep 30, 2016

adapt borg check like in PR 1554 #1663

Closed

funkyfuture added 2 commits September 30, 2016 19:48

Adds arguments to filter archives

f6b9276

These are: --sort-by, --first and --last Includes a method to obtain a list of archive infos filtered by these Archives.list: - ensure reverse is always applied - always return a list

Adds --prefix to the archives filters arguments

17f2363

- adds prefix argument to helpers.Archives.list - also renames function PrefixSpec to prefix_spec

funkyfuture added 4 commits September 30, 2016 20:14

Allows delete to be used with archive filters

f2d4d36

Adds archives filters for info

369d0a0

Adds archives filters for list

e0e9edf

Changes arg processor names to camelcase

bd7cc38

Changes on filters after feedback

089995e

ThomasWaldmann approved these changes Sep 30, 2016

View reviewed changes

enkore approved these changes Sep 30, 2016

View reviewed changes

ThomasWaldmann merged commit f38c25d into borgbackup:master Sep 30, 2016

ThomasWaldmann mentioned this pull request Oct 6, 2016

--first/last/prefix for fuse versions view #1542

Closed

Uh oh!

Uh oh!

Conversation

funkyfuture commented Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

funkyfuture commented Aug 30, 2016

Uh oh!

codecov-io commented Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 84.68% (diff: 84.74%)

Uh oh!

ThomasWaldmann Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

funkyfuture commented Aug 31, 2016

Uh oh!

ThomasWaldmann Aug 31, 2016

Choose a reason for hiding this comment

Uh oh!

funkyfuture Sep 1, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann Sep 1, 2016

Choose a reason for hiding this comment

Uh oh!

funkyfuture commented Sep 1, 2016

Uh oh!

ThomasWaldmann commented Sep 3, 2016

Uh oh!

funkyfuture commented Sep 4, 2016

Uh oh!

ThomasWaldmann commented Sep 4, 2016

Uh oh!

funkyfuture commented Sep 21, 2016

Uh oh!

enkore commented Sep 30, 2016

Uh oh!

ThomasWaldmann Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann commented Sep 30, 2016

Uh oh!

funkyfuture commented Sep 30, 2016 via email

Uh oh!

ThomasWaldmann commented Sep 30, 2016

Uh oh!

funkyfuture commented Sep 30, 2016

Uh oh!

enkore commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

funkyfuture commented Sep 30, 2016

Uh oh!

ThomasWaldmann commented Sep 30, 2016

Uh oh!

ThomasWaldmann commented Sep 30, 2016

Uh oh!

funkyfuture commented Sep 30, 2016

Uh oh!

ThomasWaldmann left a comment

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

funkyfuture commented Aug 30, 2016 •

edited

Loading

codecov-io commented Aug 30, 2016 •

edited

Loading

ThomasWaldmann Aug 30, 2016 •

edited

Loading

enkore commented Sep 30, 2016 •

edited

Loading

ThomasWaldmann commented Sep 30, 2016 •

edited

Loading