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
BF: ORA authenticated HTTP access #5026
Conversation
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.
PR #5035 should solve the issue re datalad-get
which is an artifact of running in a virtualenv.
Otherwise this change is fine with me. We loose progress reporting by the special remote that way, but supporting authenticated HTTP seems more important and breaking down download_url
in a way that allows for it is a bigger surgery.
Note, that the failure boils down to |
It intentionally does not support all of the capabilities of downloaders/providers.
Previous request result handling assumed that HTTP error codes necessarily result in exceptions, which they do not. Instead of implementing error handling per request, use an existing abstraction. Fixes dataladgh-5023 ... although it could be made better by supporting git-credentials as a credential sources in DataLad.
This uses a newly established helper (from dataladgh-5025). However, while this is working with `git annex get`, it does not with `datalad get` -- unclear why.
Rebased on top of reworked #5025 |
Codecov Report
@@ Coverage Diff @@
## maint #5026 +/- ##
==========================================
- Coverage 89.94% 89.92% -0.03%
==========================================
Files 292 292
Lines 40849 40861 +12
==========================================
+ Hits 36742 36744 +2
- Misses 4107 4117 +10
Continue to review full report at Codecov.
|
Thx @bpoldrack This is good now. |
providers = Providers.from_config_files() | ||
# not returning anything: download() seems to just echo `dest` | ||
if dest: | ||
return providers.download(url, path=str(dest), overwrite=overwrite) |
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 like a comment/code mismatch regarding return behavior.
Otherwise, it looks fine to me.
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.
Thx, removed completely now -- wasn't valid.
There is a test failure in one of the Travis jobs: |
This sits on top of #5025 (to be able to make use of the
download_url()
helper), and replaces a custom HTTP GET that is unaware of error response codes (#4980) and necessary credentials (#5024).ATM this solution is only working for
git annex get
, but notdatalad get
. No idea why.#5024 (comment)
Fixes #4980
Fixes #5024