Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul ORA remote's HTTP access to RIA stores #5459

Merged
merged 1 commit into from Aug 15, 2021

Conversation

bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented Mar 3, 2021

Restructure ORA remote's HTTPRemoteIO to remove special cases in ORA's code, where HTTP is treated somewhat differrent than SSH or local access.
For RIA stores, this leads to the ria-layout-version files being required to be served as well (which technically wasn't the case before). It also means that the request URLs now use paths for annexed objects according to the specified layout, whereas before we'd always send the same request and it was up to the server to consider its layout and deliver the right thing.

HTTP stores are validated on annex initremote/annex enableremote the same way they are when using ssh: or file: scheme URLs. Therefore, autoenabling will fail under the same conditions, making clone's reconfiguration business consistent across access methods.

Closes #5451

@bpoldrack bpoldrack marked this pull request as draft March 3, 2021 14:56
@bpoldrack bpoldrack changed the base branch from master to maint March 3, 2021 14:58
@bpoldrack bpoldrack changed the base branch from maint to master March 3, 2021 14:59
@bpoldrack bpoldrack added this to To consider in Consolidate RIA/ORA Mar 12, 2021
@bpoldrack bpoldrack moved this from To consider to Fixes in Consolidate RIA/ORA Mar 24, 2021
@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #5459 (9f85489) into master (66bea22) will decrease coverage by 2.00%.
The diff coverage is 76.55%.

❗ Current head 9f85489 differs from pull request most recent head a4b87f0. Consider uploading reports for the commit a4b87f0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5459      +/-   ##
==========================================
- Coverage   90.63%   88.63%   -2.01%     
==========================================
  Files         309      307       -2     
  Lines       42444    42454      +10     
==========================================
- Hits        38471    37628     -843     
- Misses       3973     4826     +853     
Impacted Files Coverage Δ
datalad/core/distributed/push.py 92.46% <ø> (-0.03%) ⬇️
datalad/core/local/tests/test_diff.py 97.23% <ø> (-2.31%) ⬇️
datalad/core/local/tests/test_run.py 100.00% <ø> (ø)
datalad/core/local/tests/test_save.py 97.49% <ø> (-0.54%) ⬇️
datalad/customremotes/datalad.py 39.34% <ø> (-0.98%) ⬇️
datalad/customremotes/tests/test_archives.py 89.86% <ø> (+0.60%) ⬆️
datalad/dataset/repo.py 95.38% <ø> (-0.14%) ⬇️
datalad/distributed/create_sibling_ria.py 82.65% <ø> (-0.10%) ⬇️
datalad/distributed/export_to_figshare.py 26.41% <ø> (+0.32%) ⬆️
...ad/distributed/tests/test_create_sibling_gitlab.py 100.00% <ø> (ø)
... and 153 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66bea22...a4b87f0. Read the comment docs.

@bpoldrack
Copy link
Member Author

Changed the approach and updated PR description accordingly.

@bpoldrack bpoldrack marked this pull request as ready for review July 28, 2021 23:29
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I left a few minor comments.

There should be a test though. Given that the original issue is about not failing, in case of an invallid URL such a test requires no particular infrastructure, just a random URL with no RIA store behind it.

Given the more substantial changes in this PR, the title should also be updated.

Thx!

datalad/distributed/ora_remote.py Outdated Show resolved Hide resolved
datalad/distributed/ora_remote.py Outdated Show resolved Hide resolved
datalad/distributed/ora_remote.py Outdated Show resolved Hide resolved
datalad/distributed/ora_remote.py Outdated Show resolved Hide resolved
`HTTPRemoteIO` now behaves largely just like SSH and local access,
except for being read-only. Remove special casing throughout
`RIARemote`. This means, that RIA stores are validated by the ORA
special remote over HTTP the same way as over SSH or local access.
Therefore providing the ria-layout-version files is now mandatory for
HTTP stores as well.

This patch should streamline things a bit and closes datalad#5451. Thus making
the reconfiguration business in `datalad clone` consistent across
protocols. Furthermore, since the version files are now requested and
used to construct the URLs, we can simplify server-side scripts dealing
wiht archives, etc. There's also hope that this way we can allow for a
version file to define a layout directly, instead of declaring a label
that needs to be known to released datalad, because all the version is
about is locations associated with the .git, annex/objects and archives
of datasets in the store. In theory these locations could be declared in
a JSON-record or alike in that version file, allowing store maintainers
to define their thing freely.
@bpoldrack bpoldrack changed the title BF: ORA remote didn't verify HTTP access on initremote Overhaul ORA remote's HTTP access to RIA stores Aug 4, 2021
@bpoldrack bpoldrack added the semver-minor Increment the minor version when merged label Aug 4, 2021
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes.

@mih mih merged commit 5e54de1 into datalad:master Aug 15, 2021
Consolidate RIA/ORA automation moved this from Fixes to Done Aug 15, 2021
@mih mih deleted the fix-ora-http branch August 15, 2021 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Increment the minor version when merged
Projects
Development

Successfully merging this pull request may close these issues.

enableremote ORA doesn't fail on invalid/inaccessible HTTP URL
3 participants