-
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
New S3 downloader for internal-data-pipeline #155
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
=======================================
Coverage 82.29% 82.30%
=======================================
Files 27 28 +1
Lines 819 842 +23
=======================================
+ Hits 674 693 +19
- Misses 145 149 +4 ☔ View full report in Codecov by Sentry. |
Add copyright header and spacings between import and docstring (e.g. like here: https://gitlab.cee.redhat.com/service/app-interface/-/merge_requests/99061) |
Fixed |
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.
Some nitpicks to take a look into it. Please, use pre-commit hook locally to format the code according to our "code style"
Please, add some unit test, as the code is pretty small it won't take too much
e67cb5a
to
bfac38e
Compare
bfac38e
to
6d5feea
Compare
23a222e
to
10a3382
Compare
Description
We cant use the default insights messaging S3 downloader because we need to put bucket name into the path name hence the creation of simple modification of this downloader is needed.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Testing steps
Tested localy
Checklist
make before_commit
passes