Conversation
All tests pass except test_hdf5root.py
FrancescAlted
left a comment
FrancescAlted
left a comment
There was a problem hiding this comment.
Excellent pruning work! I see that you keep your shears sharp and clean :-)
FWIW, I have some (light) comments. Also, I have run the new version locally and it runs flawlessly for what I have experienced (including HDF5 support). I am approving this, so feel free to merge when you consider this finished.
| urlbase = "https://cat2.example.com" | ||
| # ... other settings as above ... | ||
| statedir = "_caterva2/sub" | ||
| #http = "_caterva2/sub/uvicorn.socket" |
There was a problem hiding this comment.
Is that still working? If so, maybe it is worth a simple line what is that.
There was a problem hiding this comment.
Yes, they are described above:
# - statedir: the directory where the subcriber's data will be stored (default: _caterva2/sub)
# - http: where the subscriber listens to (a unix socket or a host/port) (default: localhost:8002)
# - urlbase: the base url users will use to reach the subscriber (default: http://localhost:8002)
| # src should start with a special root or known root | ||
| for src in srcs: | ||
| if not src.startswith(("@personal", "@shared", "@public")) and src not in settings.database.roots: | ||
| if not src.startswith(("@personal", "@shared", "@public")): |
There was a problem hiding this comment.
AFAIK, there is no support for other roots than these, so I wonder if this check is necessary.
There was a problem hiding this comment.
In this specific case if the src parameter has an unexpected value we send a "400 Bad Request" response, so the check is still relevant.
But there are many other places that still need clean-up, either in this PR or in a follow up. For example just below for the dst argument, there is still code for supporting the old roots, this should be change to instead return an error.
| [1, slice(None, 1), slice(0, 10), slice(10, 20), slice(None), slice(10, 20, 1)], | ||
| ) | ||
| def test_dataset_getitem_fetch(slice_, examples_dir, client): | ||
| def test_dataset_getitem_fetch(slice_, examples_dir, client, fill_public): |
There was a problem hiding this comment.
fill_public does not seem to be used here.
There was a problem hiding this comment.
The return value of fill_public is not used, but this fixture copies data (to the @public root) that is required for the test to pass.
| def test_download_public_file(examples_dir, fill_public, tmp_path): | ||
| fnames, mypublic = fill_public | ||
| for fname in fnames: | ||
| # TODO fetch (and download) of HDF5 files is not supported (gives a 500 error) |
|
I will merge this PR and create an issue with follow-up actions. |
2 participants
No description provided.