Make bob archive work remotely #617

MarcKe · 2025-02-21T13:53:21Z

relates to issue #340

MarcKe · 2025-02-21T13:53:45Z

I want to get some first feedback from your side.

codecov · 2025-02-21T13:58:08Z

Codecov Report

Attention: Patch coverage is 69.16836% with 152 lines in your changes missing coverage. Please review.

Project coverage is 88.37%. Comparing base (d7af1be) to head (ea74301).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
pym/bob/archive.py	63.05%	75 Missing ⚠️
pym/bob/webdav.py	60.89%	70 Missing ⚠️
pym/bob/cmds/archive.py	93.69%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
- Coverage   88.95%   88.37%   -0.58%     
==========================================
  Files          48       49       +1     
  Lines       15614    15887     +273     
==========================================
+ Hits        13889    14040     +151     
- Misses       1725     1847     +122

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkloetzke · 2025-02-22T11:27:24Z

Sure. First of all, this feature is very much appreciated. It's been on the TODO list for a long time. 👍

Before reviewing the code in detail, a couple of things that I noticed:

The archive command should still support working locally. That is, it should be possible to execute the command for a local directory. This is useful to run bob archive as a cron-job on the storage server.
I guess it should be possible to work on selected archive backends if there is more than one. I would even argue that it's better to require an explicit selection of the user on the desired servers and/or local directories. By doing the operation on all backends, it might lead to undesired data loss because the user was not aware it is affecting all at the same time.
That probably requires something like a name attribute for a backend. This will be handy also in the DOWNLOAD/UPLOAD messages. It was always confusion if multiple backends are configured, which one exactly had a hit or miss when building. If no name is given, you simply cannot run bob-archive on it.
Should we add a managed flag? You might not have the rights to enumerate or delete artifacts even though you can upload.

Code wise, I think the circular dependency between the webdav and archive modules needs to be properly resolved. IMHO, the webdav module should throw HTTP errors. That will require some adaptation in the archive module but this hack to resolve the circular dependency is, well, just a hack...

MarcKe · 2025-02-24T07:02:19Z

Sure. First of all, this feature is very much appreciated. It's been on the TODO list for a long time. 👍

Before reviewing the code in detail, a couple of things that I noticed:
* The archive command should still support working locally. That is, it should be possible to execute the command for a local directory. This is useful to run `bob archive` as a cron-job on the storage server.

Yes, sounds reasonable, with an option like -l/--local? Would that be ok?

* I guess it should be possible to work on selected archive backends if there is more than one. I would even argue that it's better to require an explicit selection of the user on the desired servers and/or local directories. By doing the operation on all backends, it might lead to undesired data loss because the user was not aware it is affecting all at the same time.

Sounds good to me. There could be an argument like -a/--all to just do it like I have implemented it now.

* That probably requires something like a `name` attribute for a backend. This will be handy also in the DOWNLOAD/UPLOAD messages. It was always confusion if multiple backends are configured, which one exactly had a hit or miss when building. If no name is given, you simply cannot run bob-archive on it.

I will have a look at this together with the explicit selection.

* Should we add a `managed` flag? You might not have the rights to enumerate or delete artifacts even though you can upload.

I think this is also a good idea.

Code wise, I think the circular dependency between the webdav and archive modules needs to be properly resolved. IMHO, the webdav module should throw HTTP errors. That will require some adaptation in the archive module but this hack to resolve the circular dependency is, well, just a hack...

Good idea. I will have a look at this, too.

jkloetzke · 2025-02-24T20:13:17Z

Yes, sounds reasonable, with an option like -l/--local? Would that be ok?

Yes, sounds good.

jkloetzke

This turned out to be quite a lot of comments. I certainly did not catch everything yet but I hope you bear with me...

pym/bob/webdav.py

pym/bob/archive.py

pym/bob/webdav.py

test/unit/test_archive.py

pym/bob/cmds/archive.py

test/black-box/archive/run.sh

pym/bob/input.py

pym/bob/cmds/archive.py

MarcKe · 2025-03-04T08:30:48Z

This turned out to be quite a lot of comments. I certainly did not catch everything yet but I hope you bear with me...

No, thank you, this is great.

pym/bob/archive.py

pym/bob/webdav.py

pym/bob/archive.py

pym/bob/webdav.py

pym/bob/archive.py

pym/bob/cmds/archive.py

pym/bob/webdav.py

pym/bob/archive.py

pym/bob/cmds/archive.py

MarcKe · 2025-03-11T15:46:47Z

I think I have managed to go through all your remarks.

currently working with local and http/webdav repositories

added argument -l/--local to archive cmd that will execute the archive cmd in the current directory

…override -n/--names: a comma-separated list of archives to work on (names from user config) -a/--all: use all suitable archives from user config

used to specify archives that support deleting/iterating files, required for archive cmd

jkloetzke · 2025-03-12T21:23:11Z

Thanks. I'll have a look on the weekend (hopefully).

jkloetzke

That looks quite good already. Would it be possible to add some testing for the Webdav parts? Maybe (re)use the HTTP mock server from the unit tests to have some minimally responsive Webdav server? Otherwise this is basically all untested and I fear it will break unnoticed whenever we touch the archive stuff...

jkloetzke · 2025-03-15T20:29:55Z

pym/bob/cmds/archive.py

@@ -23,15 +22,19 @@
 class ArchiveScanner:
    CUR_VERSION = 2


We must bump the version because the schema changed.

jkloetzke · 2025-03-15T20:44:30Z

pym/bob/cmds/archive.py

@@ -500,6 +500,12 @@ def doArchive(argv, bobRoot):
 """.format(subHelp))
    parser.add_argument("-l", "--local", action='store_true',
                        help="ignore the archive configuration and work on the current directory")
+    parser.add_argument("-a", "--all", action='store_true',


I think you can use a add_mutually_exclusive_group for -l, -a and -b. That way, only one of them can be given and we do not have to define a precedence.

MarcKe force-pushed the archive_work_remotely branch from cd10be3 to 832f424 Compare February 25, 2025 10:20

jkloetzke reviewed Mar 3, 2025

View reviewed changes

MarcKe commented Mar 5, 2025

View reviewed changes

MarcKe force-pushed the archive_work_remotely branch from 832f424 to 5f86327 Compare March 11, 2025 15:41

MarcKe added 6 commits March 12, 2025 08:07

introduce webdav class

be2c3fb

archive: allow command to work remotely

41391fc

currently working with local and http/webdav repositories

archive: allow working on current directory

6a722aa

added argument -l/--local to archive cmd that will execute the archive cmd in the current directory

archive: add optional name for archive backend

6d73428

archive: add argument to explicitly specify archives to work on with …

90dc276

…override -n/--names: a comma-separated list of archives to work on (names from user config) -a/--all: use all suitable archives from user config

archive: add managed flag

ea74301

used to specify archives that support deleting/iterating files, required for archive cmd

MarcKe force-pushed the archive_work_remotely branch from 5f86327 to ea74301 Compare March 12, 2025 07:07

jkloetzke reviewed Mar 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make bob archive work remotely #617

Make bob archive work remotely #617

MarcKe commented Feb 21, 2025

MarcKe commented Feb 21, 2025

codecov bot commented Feb 21, 2025 •

edited

Loading

jkloetzke commented Feb 22, 2025

MarcKe commented Feb 24, 2025 •

edited

Loading

jkloetzke commented Feb 24, 2025

jkloetzke left a comment

MarcKe commented Mar 4, 2025

MarcKe commented Mar 11, 2025

jkloetzke commented Mar 12, 2025

jkloetzke left a comment

jkloetzke Mar 15, 2025

jkloetzke Mar 15, 2025

Make bob archive work remotely #617

Are you sure you want to change the base?

Make bob archive work remotely #617

Conversation

MarcKe commented Feb 21, 2025

MarcKe commented Feb 21, 2025

codecov bot commented Feb 21, 2025 • edited Loading

Codecov Report

jkloetzke commented Feb 22, 2025

MarcKe commented Feb 24, 2025 • edited Loading

jkloetzke commented Feb 24, 2025

jkloetzke left a comment

Choose a reason for hiding this comment

MarcKe commented Mar 4, 2025

MarcKe commented Mar 11, 2025

jkloetzke commented Mar 12, 2025

jkloetzke left a comment

Choose a reason for hiding this comment

jkloetzke Mar 15, 2025

Choose a reason for hiding this comment

jkloetzke Mar 15, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2025 •

edited

Loading

MarcKe commented Feb 24, 2025 •

edited

Loading