Skip to content

Support for date-based matching during archive listing - #7272

Merged
ThomasWaldmann merged 7 commits into
borgbackup:masterfrom
Michael-Girma:issue-7062
Jan 23, 2023
Merged

Support for date-based matching during archive listing#7272
ThomasWaldmann merged 7 commits into
borgbackup:masterfrom
Michael-Girma:issue-7062

Conversation

@Michael-Girma

Copy link
Copy Markdown
Contributor

Support for date-matching during archive listing (fix for #7062)

  • Added arguments --older --newer --newest --oldest to argparser
  • Added parser to detect relative time markers i.e. 1d/1m
  • Added an in-house month-offseter
  • Edited manifest to initially filter by date if markers are present
  • Added test cases to test date matching arguments

---
- Added arguments --older --newer --newest --oldest to argparser
- Added parser to detect relative time markers i.e. 1d/1m
- Added an in-house month-offseter
- Edited manifest to initially filter by date if markers are present
- Added test cases to test date matching arguments
@Michael-Girma

Michael-Girma commented Jan 16, 2023

Copy link
Copy Markdown
Contributor Author

@ThomasWaldmann Made my first pass of changes and added a test case for it. The output seems to reflect the specified archives, which are the only ones being analyzed. However, I keep getting an "{num} orphaned objects found!". Not sure if this is a problem with how I created the archives in my test. All other test cases seem to work fine. Any thoughts?

Comment thread src/borg/helpers/time.py Outdated
Comment thread src/borg/helpers/time.py Outdated
Comment thread src/borg/helpers/time.py Outdated
Comment thread src/borg/manifest.py Outdated
Comment thread src/borg/manifest.py
Comment thread src/borg/testsuite/archiver/__init__.py Outdated
Comment thread src/borg/testsuite/archiver/check_cmd.py
@ThomasWaldmann

ThomasWaldmann commented Jan 16, 2023

Copy link
Copy Markdown
Member

Also:

  • run black . in the toplevel dir of the repo to get the desired automatic formatting we use.
  • run tox -e flake8 to check if there is anything else

You can ignore the windows CI failures for now, we are still working on fixing the windows tests.

@ThomasWaldmann

Copy link
Copy Markdown
Member

About the orphaned objects: interesting, guess I will need to locally run this. Usually borg should not create orphans.

---
- Added validators for argparse to reject unrecognized relative time markers
- Calculate relative offset now raises exception on invalid markers
- Match-archives now preceeds date-based filtering
- Cleaned up regex references in time helper
@Michael-Girma

Michael-Girma commented Jan 17, 2023

Copy link
Copy Markdown
Contributor Author

@ThomasWaldmann I've gone through the requests and resolved them except for the shutil.rmtree(self.repository_path) in the tests. I've run black and tox to format the code as well. Please review when possible.

Comment thread src/borg/helpers/__init__.py Outdated
Comment thread src/borg/helpers/parseformat.py Outdated
Comment thread src/borg/helpers/time.py Outdated
Comment thread src/borg/manifest.py Outdated
Comment thread src/borg/helpers/time.py Fixed
---
- Fixed inefficient regex for relative time marker matching
- Fixed import formatting
- Renamed 'dates' to 'ts' in calculate_relative_offset
- Modified relative time marker validator to match using single regex
@Michael-Girma

Copy link
Copy Markdown
Contributor Author

Applied changes requested above. Please review when possible.

Comment thread src/borg/helpers/time.py Outdated
Comment thread src/borg/helpers/time.py Outdated
Comment thread src/borg/helpers/time.py Outdated
Comment thread src/borg/helpers/time.py Outdated
Comment thread src/borg/testsuite/archiver/__init__.py Outdated
- Modified create_src_archive to branch when timestamp flag is present
@Michael-Girma

Copy link
Copy Markdown
Contributor Author

Renamed vars and refactored the offset calculation to remove code duplication. Modified the create_src_archive as well. Please review when you can

@ThomasWaldmann

Copy link
Copy Markdown
Member

The linters are unhappy, see my previous comments about black/flake8.

Comment thread src/borg/archive.py
Comment thread src/borg/archiver/_common.py
Comment thread src/borg/manifest.py Outdated
Comment thread src/borg/testsuite/archiver/check_cmd.py
@ThomasWaldmann

ThomasWaldmann commented Jan 19, 2023

Copy link
Copy Markdown
Member

@Michael-Girma btw, would you like to earn a bounty for this?

we don't have much funds, but I guess I could put a USD 50 bounty on solving this ticket (we use bountysource.com).

@Michael-Girma

Copy link
Copy Markdown
Contributor Author

@Michael-Girma btw, would you like to earn a bounty for this?

we don't have much funds, but I guess I could put a USD 50 bounty on solving this ticket (we use bountysource.com).

That would be awesome. : ))

@Michael-Girma

Michael-Girma commented Jan 20, 2023

Copy link
Copy Markdown
Contributor Author

Ahh forgot to run the linters before committing. What are your thoughts on utilizing the pre-commit hook for linting?

---
- Updated check/list functions to have same order of older/newer ...
- Updated archive docstring to reflect parameters
- Updated manifest docstring to reflect order of exec of parameters
- Updated argparser group help description
@ThomasWaldmann

Copy link
Copy Markdown
Member

@Michael-Girma bounty added (see ticket). about pre-commit: open a ticket about it, so we can discuss there.

Comment thread src/borg/archiver/_common.py Outdated
Comment thread src/borg/manifest.py
Comment thread src/borg/testsuite/archiver/check_cmd.py Outdated
@ThomasWaldmann

Copy link
Copy Markdown
Member

Test fails:

E           TypeError: calculate_relative_offset() got an unexpected keyword argument 'from_date'

---
- Revisited argparser group for newer / older
- Revisited docstring for archives list method
@ThomasWaldmann

ThomasWaldmann commented Jan 23, 2023

Copy link
Copy Markdown
Member
        output = self.cmd(
            f"--repo={self.repository_location}", "check", "-v", "--archives-only", "--newest=1m", exit_code=0
        )
>       self.assert_in("archive2", output)
E       AssertionError: 'archive2' not found in 'Starting archive consistency check...\nAnalyzing archive archive3 (1/1)\nOrphaned objects check skipped (needs all archives checked).\nArchive consistency check complete, no problems found.\n'

.tox/py39-fuse2/lib/python3.9/site-packages/borg/testsuite/archiver/check_cmd.py:78: AssertionError

BTW, you can run the tests also locally before pushing: tox -e py39

@Michael-Girma

Copy link
Copy Markdown
Contributor Author

Keep getting a

.tox/py39/lib/python3.9/site-packages/borg/remote.py:40: in <module>
    BORG_VERSION = parse_version(__version__)
.tox/py39/lib/python3.9/site-packages/borg/version.py:24: in parse_version
    raise ValueError("Invalid version string %s" % version)
E   ValueError: Invalid version string 0.1.dev7507+gf88cc5a

when trying to run tests locally.

@ThomasWaldmann

Copy link
Copy Markdown
Member

Still draft (anything left to do?) oder "ready for review"?

@Michael-Girma
Michael-Girma marked this pull request as ready for review January 23, 2023 13:11
@ThomasWaldmann

Copy link
Copy Markdown
Member

@Michael-Girma before running the tests:

# have a full git clone and be in the toplevel project dir
python setup.py clean clean2
pip install -r requirements.d/development.txt
pip install -e .

(and before that, follow the docs and have all OS level packages installed, like libs and header files.)

@codecov-commenter

codecov-commenter commented Jan 23, 2023

Copy link
Copy Markdown

Codecov Report

Merging #7272 (f88cc5a) into master (ca1f128) will decrease coverage by 0.12%.
The diff coverage is 79.54%.

@@            Coverage Diff             @@
##           master    #7272      +/-   ##
==========================================
- Coverage   83.49%   83.37%   -0.12%     
==========================================
  Files          67       67              
  Lines       11551    11651     +100     
  Branches     2095     2123      +28     
==========================================
+ Hits         9644     9714      +70     
- Misses       1355     1367      +12     
- Partials      552      570      +18     
Impacted Files Coverage Δ
src/borg/archiver/check_cmd.py 73.80% <ø> (ø)
src/borg/helpers/parseformat.py 89.70% <33.33%> (+0.64%) ⬆️
src/borg/archive.py 82.40% <70.00%> (-0.67%) ⬇️
src/borg/helpers/time.py 80.43% <77.77%> (-1.39%) ⬇️
src/borg/archiver/_common.py 89.78% <80.00%> (-0.67%) ⬇️
src/borg/helpers/__init__.py 100.00% <100.00%> (ø)
src/borg/manifest.py 93.90% <100.00%> (+0.68%) ⬆️
src/borg/upgrade.py 72.44% <0.00%> (-2.00%) ⬇️
src/borg/locking.py 85.57% <0.00%> (-1.76%) ⬇️
src/borg/archiver/info_cmd.py 89.18% <0.00%> (-1.06%) ⬇️
... and 9 more

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

@ThomasWaldmann ThomasWaldmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@ThomasWaldmann

ThomasWaldmann commented Jan 23, 2023

Copy link
Copy Markdown
Member

#7272 (comment)

how can i reproduce these? did you still get that after fixing the full/partial check (self.check_all) flag?

@Michael-Girma

Copy link
Copy Markdown
Contributor Author

#7272 (comment)

how can i reproduce these? did you still get that after fixing the full/partial check (self.check_all) flag?

Resetting my env, will test in a few

@ThomasWaldmann

ThomasWaldmann commented Jan 23, 2023

Copy link
Copy Markdown
Member

Working on a more condensed commit comment for the squash commit. How about:

check --archives: add --newer/--older/--newest/--oldest, fixes #7062

Options accept a timespan, like Nd for N days or Nm for N months.

Use these to do date-based matching on archives and only check some of them,
like: borg check --archives --newer=1m --newest=7d

Author: Michael Deyaso <mdeyaso@fusioniq.io>

@ThomasWaldmann

Copy link
Copy Markdown
Member

OK, CI looks good (windows CI failures are expected and not due to this PR).

@Michael-Girma

Copy link
Copy Markdown
Contributor Author

Tests are all green locally as well. Thanks for the help with setting up my env. The squash-commit comment looks good to me.

@ThomasWaldmann
ThomasWaldmann merged commit b2654bc into borgbackup:master Jan 23, 2023
@ThomasWaldmann

Copy link
Copy Markdown
Member

Merged, thanks! Now claim the bounty...

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