-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add S3 downloader #124
Add S3 downloader #124
Conversation
Codecov Report
@@ Coverage Diff @@
## main #124 +/- ##
==========================================
+ Coverage 88.61% 89.93% +1.32%
==========================================
Files 22 24 +2
Lines 4425 5066 +641
==========================================
+ Hits 3921 4556 +635
- Misses 504 510 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…ves into issue123-s3downloader
…ves into issue123-s3downloader
…ves into issue123-s3downloader
Tests are failing, but this time something else. |
If I run the failing functional test locally it passes. If I run all the functional tests, they pass. If I run all the tests, I get failures in
My first guess is some fixture with |
Umm, those two failing tests in |
Ah, I guess the |
yay |
I think I also need to review how credentials are used here. Maybe I get time later today to have a look |
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.
Lot of work, nice! I think this looks good, but have not gone in very deep detail. Added a few small comments
The tests failed again, but I think its OK. Do want to have another round of review? |
While testing to see what's happening with the functional test failure, I see another one locally. Sorting this will need help from @mraspaud |
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.
LGTM
Add module/daemon for s3 download
AUTHORS.md
if not there already