-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/sda download #392
Feature/sda download #392
Conversation
- Download a file and check the path and the file itself - Remove s3cmd-admin because it is the same with the directS3 - Renamed the s3cmd.conf to template and then different tests can create a config and add different tokens - Fixed the url check and fail if there is not protocol (https or http)
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
==========================================
+ Coverage 40.48% 42.25% +1.77%
==========================================
Files 11 12 +1
Lines 1539 1678 +139
==========================================
+ Hits 623 709 +86
- Misses 837 878 +41
- Partials 79 91 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The app should only have one download entry
- The static tests mixes error catching and assertions.
- There is a lot of excess whitespace in the form on empty lines directly after function and variable declarations.
- the cli now checks the format of the first part of the filepath and if it is a userid like does not include it - add a userid in the filepath in integration setup
Co-authored-by: Joakim Bygdell <joakim.bygdell@nbis.se>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! And very nice that you provided an easy-to-run test setup!
Co-authored-by: Malin Klang <ahlberg.malin@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great! Some minor comments regarding documentation :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works very well when I tested. A general feeling is that the naming of sub commands download
and sda-download
does not clearly distinguish methods for downloading.
I don't have any good suggestions neither. Maybe
s3-download: by downloading from a S3 bucket
api-download: by using the download API
Co-authored-by: Alex Aperis <76202622+aaperis@users.noreply.github.com>
Update the download environment variables for working with the latest image
Related issue(s) and PR(s)
This PR closes #372.
Description
A new feature in the sda-cli is implemented to download files from the download service endpoint by giving the dataset stable_id and the inbox path of the file without the uploader's user id
How to test
By testing that just run the setup script from the root folder:
bash .github/integration/setup/setup.sh
and when it is done one can download the file by running:
./sda-cli sda-download -config testing/s3cmd-download.conf -dataset https://doi.example/ty009.sfrrss/600.45asasga -url http://localhost:8080 -outdir test-download main/subfolder/dummy_data.c4gh
Then check if the file
test-download/main/subfolder/dummy_data
exists and if the result by runninghead test-download/main/subfolder/dummy_data
is "THIS FILE IS JUST DUMMY DATA"