whole-tale / girder_wholetale

Girder plugin providing basic Whole Tale functionality
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

Deriva support #510

Closed hategan closed 2 years ago

hategan commented 2 years ago

This, together with the corresponding dms PR (https://github.com/whole-tale/girder_wt_data_manager/pull/51), adds:

I don't have a dashboard running, and I don't really understand the integrations thing (e.g., at what point in the integrations flow does the import actually happen?), so I tried to copy what I saw in other cases. That said, this can be tested by invoking the familiar importData endpoint with a link to a deriva bdbag, such as the one appearing in https://identifiers.fair-research.org/hdl:20.500.12633/11RHwdYqWNBZL, creating a session with the resulting folder, and mounting that session with girderfs/wt_dms_fs. That's somewhat tedious, but I don't know of a better solution.

codecov[bot] commented 2 years ago

Codecov Report

Merging #510 (9788b95) into master (0711238) will decrease coverage by 1.11%. The diff coverage is 63.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
- Coverage   92.89%   91.78%   -1.12%     
==========================================
  Files          55       58       +3     
  Lines        4294     4443     +149     
==========================================
+ Hits         3989     4078      +89     
- Misses        305      365      +60     
Impacted Files Coverage Δ
server/__init__.py 90.20% <42.85%> (-3.05%) :arrow_down:
server/lib/deriva/integration.py 45.16% <45.16%> (ø)
server/lib/deriva/auth.py 47.36% <47.36%> (ø)
server/lib/resolvers.py 82.08% <56.52%> (-13.57%) :arrow_down:
server/lib/deriva/provider.py 68.00% <68.00%> (ø)
server/lib/bdbag/bdbag_provider.py 90.90% <86.66%> (-0.93%) :arrow_down:
server/constants.py 89.13% <100.00%> (+0.75%) :arrow_up:
server/lib/__init__.py 98.07% <100.00%> (+0.07%) :arrow_up:
server/rest/integration.py 100.00% <100.00%> (ø)

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 0711238...9788b95. Read the comment docs.

Xarthisius commented 2 years ago

@hategan ^^ just a friendly ping.

hategan commented 2 years ago

Ok, so I rebased this (and the DMS) on auth_requests and used that scheme for the headers. It wasn't entirely smooth, since the DERIVA token does not have the resources_server pointing to the FQDN, so I had to override the default mechanism.

@Xarthisius otherwise, auth_requests seems like a nice way of doing things and I approve of that general message. There were some inconsistencies in the verificators (some inherit from Verificator, some don't), but I guess that's fine in Python.

I also added settings wherever there were hardcoded URLs, as @craig-willis suggested. It is somewhat clear that, should this be adopted by the larger DERIVA community, there will be multiple URLs.

I did a manual test. At first I got an auth error inside the DMS. We have no way of reporting these to the user or offering the chance to take some pre-defined corrective action, but that's another story. It worked after I logged in and out of both deriva and Globus.