Extends delete command with --oldest & --latest options by funkyfuture · Pull Request #1369 · borgbackup/borg

funkyfuture · 2016-07-23T19:09:23Z

i added a --latest and --oldest option to simply delete the newest resp. oldest backup from a repository. eg, it comes handy in bash scripts:

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

alternatively, it could be implemented as --last n and --first n where n defaults to 1.

enkore · 2016-07-23T21:40:33Z

+
+        if args.oldest or args.latest:
+            if args.location.archive:
+                logger.warning('The options --oldest and --latest have no effect on archive targets.')


This branch should abort to avoid unintentionally deleting stuff.

enkore · 2016-07-23T21:43:19Z

Interesting feature. #128 is kinda related, maybe we can factor this into a more generic version that is easier to apply to other commands (e.g. an Archiver method that sets args.location.archive from --latest/--oldest flags). Looking at the code it wouldn't change too much. What do you think?

funkyfuture · 2016-07-23T21:51:31Z

sure, more generic is always reasonable. what about these properties / methods?

latest_archive -> an archive name or None
last_archives(n) -> a possibly empty list of archive names, returns all archive names of the repo if n is greater than the number of archives
oldest_archive (equivalent)
oldest_archives(n) (equivalent)

funkyfuture · 2016-07-23T21:55:11Z

or should these rather be attributes of helpers.Manifest?

funkyfuture · 2016-07-23T23:34:23Z

i followed these thoughts by implementing with this outcome:

the functionality of Manifest.list_archive_infos is flexible enough to avoid overhead in the deletion procedure. don't know if they could be usable in some context. so i'd rather add an Archiver method that takes args and repository or manifest to figure out a scope for similar uses of these arguments. info, list and check seem to be the only candidates for it.

i ended up with something that is not exactly what #128 proposes. it's rather a slicing now, so --latest 1 instead of just --latest, but i see no disadvantage here.

ThomasWaldmann · 2016-07-24T10:15:45Z

I also did implement some --last N stuff for borg check already, see the code there.

If generalizing that, we need to be careful with defaults (if that code includes handling of that).
For borg check of course it makes sense to have a default of "check all", but not for borg delete.

ThomasWaldmann · 2016-07-24T10:25:43Z

+                if self.exit_code:
+                    break
+            else:
+                logger.error('There are no archives.')


doesn't it always end in here, if there is no exit_code > 0 (warning or error) above?
if we successfully deleted all archives, why does it emit this error? (assuming we wanted to delete all archives)

no, this is only reached if archives is empty.

>>> for i in range(3): ... print(i) ... else: ... print("didn't break out of the loop") ... 0 1 2 didn't break out of the loop

thanks for reminding me that i need to get rid of that misconception i have about for ... else.

funkyfuture · 2016-07-24T10:52:22Z

If generalizing that, we need to be careful with defaults (if that code includes handling of that).
For borg check of course it makes sense to have a default of "check all", but not for borg delete.

i think the simplest would be to allow --latest N and --oldest N only on repositories, thus parse_args should check that the target is no archive.

though i would prefer a default of 1 for each argument, problems arise as the code can't figure out whether and which flag was actually provided by the user. a clever idea to deal with that would be interesting, but still one would need -- after an optional argument with an omitted value.

as of now, either of both flags must be used with an explicit argument, which makes it quiet sane, imo.

this distinguishes it from the --last option. hence i wouldn't use the same name.

funkyfuture · 2016-07-24T18:21:37Z

i tend to move argparsing code to an own module, especially as Archiver.build_parser is hard to maintain. any objections?

ThomasWaldmann · 2016-07-24T20:37:43Z

we already thought about that, but delayed it because we want to maintain multiple branches concurrently and be able to merge changes in archiver.py without troubles. and we shouldn't do big src cleanup in 1.0-maint.

funkyfuture · 2016-08-13T12:59:32Z

okay, i can need some feedback here.

info and list are now also usable with something i would term slice selection for now, but would prefer something clearer.

the rationale for using --oldest and --lastest as argument names is their more specific meaning concerning time, as the antonyms --first and --last may also be used in another sorting contexts.

i propose to refactor check similarly by moving the selection and dispatching logic to Archiver and making the --last option an alias of --latest.

ThomasWaldmann · 2016-08-20T19:58:17Z

+        """Delete multiple archives"""
+        manifest, key = Manifest.load(repository)
+        archives = self._get_archives_slice(args, manifest)
+        for i, archive in enumerate(archives):


... enumerate(archives, 1) (and remove the +1 below)

ThomasWaldmann · 2016-08-20T20:49:42Z

Just an idea: you cleaned up the toplevel dispatch method to be shorter and just call the 3 _xxxx methods for the 3 cases archive, archives, repo. They could be also functions INSIDE the dispatch method, not methods of Archiver. As you prefixed them with underscore, you indicate "don't call me" somehow anyway, so they could be also private functions defined inside the method?

ThomasWaldmann · 2016-08-20T20:49:57Z

Can you rebase on current master?

ThomasWaldmann · 2016-08-20T21:04:24Z

An idea about #1369 (comment):

Maybe that is the reason why we rather want to use first and last (not oldest and latest).

We could just document, that sort order is by-timestamp (so first means oldest and last means latest), but later, if we feel the need, we could also sort by other criterial (using a sort-by option) and still use the same first/last option to give the index.

Not sure if we ever need it, currently there is only timestamp and name that somehow qualify for sorting, but selecting the first/last N of a list sorted by name - not sure if it makes sense.

funkyfuture · 2016-08-21T13:35:24Z

As you prefixed them with underscore, you indicate "don't call me" somehow anyway, so they could be also private functions defined inside the method?

whenever i come across such nested callables, i experience a great loss of readability. flat is better than nested. if you really want it like this, tell me. but i really consider it a bad practice.

Not sure if we ever need it, currently there is only timestamp and name that somehow qualify for sorting, but selecting the first/last N of a list sorted by name - not sure if it makes sense.

to be consistent i would also add a --sort-by defaulting to time. this may seem overengineered atm, but as a set of arguments with --first and --last it makes sense.

enkore · 2016-08-21T13:39:24Z

Another point about nesting callables is that the namespace inside a callable is anonymous for all practical purposes, so they would be unreachable for testing.

ThomasWaldmann · 2016-08-21T17:22:57Z

Related: #128

- also ensures that `Archives.list` returns a list

also renames function PrefixSpec to prefix_spec

funkyfuture · 2016-08-22T21:21:04Z

aigh't, i added the --sort-by argument and grabbed some low hanging fruits on the way.

codecov-io · 2016-08-22T23:00:53Z

Current coverage is 84.43% (diff: 93.20%)

Merging #1369 into master will decrease coverage by 0.87%

@@             master      #1369   diff @@
==========================================
  Files            18         18          
  Lines          6031       6115    +84   
  Methods           0          0          
  Messages          0          0          
  Branches       1024       1042    +18   
==========================================
+ Hits           5145       5163    +18   
- Misses          638        697    +59   
- Partials        248        255     +7

Powered by Codecov. Last update a54205d...e3b2558

refactors ArchiveChecker to be used as context manager

ThomasWaldmann · 2016-08-23T21:27:57Z

                break
            if len(archives) - i > 1:
-                write('\n')
+                write()


ehrm, guess you need to keep the \n here. write() does not write one by default, like print() does.

ThomasWaldmann · 2016-08-23T22:00:34Z

this PR is a bit hard to review. single changesets seem to be incomplete, reviewing the overall changes is way to much.

ThomasWaldmann · 2016-08-23T22:04:01Z

Can you redo your changes and:

do not do any unrelated changes, no cleanup, no reformatting
have each changeset self-contained, complete and focussed on some topic that matches the commit comment?
if you did fixup commits, can you merge them with the commits they fixed?

funkyfuture · 2016-08-24T13:35:25Z

sorry for the mess.

if you did fixup commits, can you merge them with the commits they fixed?

that's the cause for the mixups in the commits. i'll see to consolidate and give a notice.

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

closing in favor of #1554

enkore reviewed Jul 23, 2016
View reviewed changes

ThomasWaldmann reviewed Jul 24, 2016
View reviewed changes

ThomasWaldmann reviewed Aug 20, 2016
View reviewed changes

Extends delete cmd w/ --oldest & --latest

62098af

funkyfuture added 7 commits August 22, 2016 22:53

Uncomplexifies Archiver.do_delete

f761ade

Makes delete --oldest and --latest take a quantity

baa59df

feedback changes

0b50b7d

Adds arcives slice selction for info and list

f1c8e84

feedback changes

b8c6604

Adds --sort-by option

ec01058

- also ensures that `Archives.list` returns a list

Adds prefix argument to helpers.Archives.list

8f20aee

also renames function PrefixSpec to prefix_spec

funkyfuture added 5 commits August 23, 2016 22:09

Implements check usage with --first and --last

303c479

refactors ArchiveChecker to be used as context manager

Tests and fixes for delete usage w/ archives filter

ef923f9

Tests for archives filter arguments

0403e44

Assorted cleanup

e3b2558

Simplifies --sort-by parsing

3bda57e

ThomasWaldmann reviewed Aug 23, 2016
View reviewed changes

ThomasWaldmann mentioned this pull request Aug 27, 2016

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

Closed

funkyfuture mentioned this pull request Aug 30, 2016

Archives filters #1554

Merged

funkyfuture closed this Aug 30, 2016

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

enkore Jul 23, 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

funkyfuture commented Jul 23, 2016

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ThomasWaldmann Jul 24, 2016

Choose a reason for hiding this comment

Uh oh!

funkyfuture Jul 24, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann Jul 24, 2016

Choose a reason for hiding this comment

Uh oh!

funkyfuture Jul 24, 2016

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ThomasWaldmann commented Jul 24, 2016

Uh oh!

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

Uh oh!

Uh oh!

ThomasWaldmann Aug 20, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann commented Aug 20, 2016

Uh oh!

ThomasWaldmann commented Aug 20, 2016

Uh oh!

ThomasWaldmann commented Aug 20, 2016

Uh oh!

funkyfuture commented Aug 21, 2016

Uh oh!

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

Uh oh!

Uh oh!

ThomasWaldmann commented Aug 21, 2016

Uh oh!

funkyfuture commented Aug 22, 2016

Uh oh!

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

Uh oh!

Current coverage is 84.43% (diff: 93.20%)

Uh oh!

ThomasWaldmann Aug 23, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann commented Aug 23, 2016

Uh oh!

ThomasWaldmann commented Aug 23, 2016

Uh oh!

funkyfuture commented Aug 24, 2016

Uh oh!

funkyfuture commented Aug 30, 2016

Uh oh!

funkyfuture commented Jul 23, 2016 •

edited

Loading

enkore Jul 23, 2016 •

edited

Loading

enkore commented Jul 23, 2016 •

edited

Loading

funkyfuture commented Jul 23, 2016 •

edited

Loading

funkyfuture commented Jul 23, 2016 •

edited

Loading

ThomasWaldmann commented Jul 24, 2016 •

edited

Loading

funkyfuture commented Jul 24, 2016 •

edited

Loading

funkyfuture commented Jul 24, 2016 •

edited

Loading

funkyfuture commented Aug 13, 2016 •

edited

Loading

enkore commented Aug 21, 2016 •

edited

Loading

codecov-io commented Aug 22, 2016 •

edited

Loading