search

whole-tale / girder_wholetale

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

Include rruns during tale import #517

Closed Xarthisius closed 2 years ago

Xarthisius commented 2 years ago

...

How to test?

  1. Prepare a bag with multiple rruns.
  2. Import: 
    curl -X POST \
--header 'Girder-Token: ...' \
-H "Content-Type:application/zip" \
--data-binary "@61f18414fdfd5791fbb61b7b.zip" \
-w '\n' \
'https://girder.local.wholetale.org/api/v1/tale/import'

TODO

Notes

  1. We don't have a run status now. I'm assuming it was successful.
  2. Ctime/mtime on actual files in both version and runs folders are not preserved. I could potentially coerce it to whatever modifiedDate for each of those is.
codecov[bot] commented 2 years ago

Codecov Report

Merging #517 (ff7d2b5) into master (7d101a1) will increase coverage by 0.30%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage   91.84%   92.15%   +0.30%     
==========================================
  Files          58       58              
  Lines        4450     4460      +10     
==========================================
+ Hits         4087     4110      +23     
+ Misses        363      350      -13
Impacted Files Coverage Δ
server/lib/manifest.py 92.97% <ø> (ø)
server/tasks/import_tale.py 100.00% <100.00%> (+1.20%) :arrow_up:
server/lib/manifest_parser.py 87.16% <0.00%> (+8.10%) :arrow_up:

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 7d101a1...ff7d2b5. Read the comment docs.

craig-willis commented 2 years ago

Looks good, thank you.

Xarthisius commented 2 years ago

Looks good, thank you.

  • Status: I can either restrict to successful or add the status to the manifest or inside the run workspace (why is it a level up anyway?) Any preference?

As per dev call on 01/31 we agreed upon adding it as an attribute to manifest.json.

  • Ctime/mtime: Seems like overkill, but couldn't we add these to the manifest?

This: https://github.com/girder/girder/blob/0766ba8e7f9b25ce81e7c0d19bd343479bceea20/girder/utility/ziputil.py#L137 prevents it working out of the box. While it's not mutually exclusive with adding that data to manifest.json I'd prefer zip to contain proper ctime/mtime. Update: I just realized that both zip and manifest.json contains info only about files, not directories...