Skip to content
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

No longer use old-style Runner in customremotes #5142

Merged
merged 4 commits into from Nov 12, 2020

Conversation

mih
Copy link
Member

@mih mih commented Nov 11, 2020

Ping #5003

Use the runner provided by the repo instance instead.
This does not yet address the tests that specifically touch the protocol
test features of the old runner. They shall be removed, once the runner
is removed itself.
@mih mih added this to the 0.14.0 milestone Nov 11, 2020
@mih mih mentioned this pull request Nov 11, 2020
11 tasks
@mih mih modified the milestone: 0.14.0 Nov 11, 2020
["git", "annex", "get", "--json", "--json-progress", "--key", akey],
protocol=None,
cwd=self.path,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change leads to a protocol error.

demo
#!/bin/sh

set -eu

cd "$(mktemp -d "${TMPDIR:-/tmp}"/dl-XXXXXXX)"
datalad create
datalad download-url --archive \
        https://github.com/datalad/datalad-ukbiobank/archive/0.3.tar.gz
git annex drop --all
datalad get datalad-ukbiobank-0.3/README.md

On base (8081fcf):

[...]
[INFO] To obtain some keys we need to fetch an archive of size 51.5 kB 
[INFO] PROGRESS-JSON: {"command":"get","note":"from web...\nchecksum...","success":true,"input":[],"key":"MD5E-s51496--03f0fc3fa1ded1efbffbf0f0b435ef6f.tar.gz","file":null} 
get(ok): datalad-ukbiobank-0.3/README.md (file) [from datalad-archives...]

After this PR:

[...]
[INFO] To obtain some keys we need to fetch an archive of size 51.5 kB 
get(ok): datalad-ukbiobank-0.3/README.md (file) [external special remote protocol error, unexpectedly received "{"byte-progress":34861,"action":{"command":"get","note":"from web...","input":[],"key":"MD5E-s51496--03f0fc3fa1ded1efbffbf0f0b435ef6f.tar.gz","file":null},"total-size":51496,"percent-progress":"67.7%"}" (unable to parse command)
unable to use special remote due to protocol error]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arggh, true. Thanks for the demo! I have pushed a fix that adds a protocol to handle the JSON output.

@codecov
Copy link

codecov bot commented Nov 12, 2020

Codecov Report

Merging #5142 (d11cdb0) into master (8081fcf) will decrease coverage by 0.05%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5142      +/-   ##
==========================================
- Coverage   89.58%   89.53%   -0.06%     
==========================================
  Files         299      299              
  Lines       42091    42107      +16     
==========================================
- Hits        37709    37702       -7     
- Misses       4382     4405      +23     
Impacted Files Coverage Δ
datalad/customremotes/base.py 87.42% <0.00%> (-0.08%) ⬇️
datalad/customremotes/tests/test_datalad.py 96.07% <ø> (ø)
datalad/customremotes/archives.py 91.90% <100.00%> (-0.05%) ⬇️
datalad/customremotes/tests/test_archives.py 89.80% <100.00%> (-0.13%) ⬇️
datalad/downloaders/tests/test_http.py 88.65% <0.00%> (-2.84%) ⬇️
datalad/plugin/wtf.py 82.40% <0.00%> (-1.20%) ⬇️
datalad/plugin/addurls.py 99.00% <0.00%> (-0.75%) ⬇️
datalad/downloaders/base.py 80.70% <0.00%> (-0.36%) ⬇️
datalad/support/annexrepo.py 88.88% <0.00%> (-0.25%) ⬇️
datalad/distributed/ora_remote.py 31.19% <0.00%> (-0.06%) ⬇️
... and 6 more

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 8081fcf...558da2a. Read the comment docs.

mih added a commit to mih/datalad that referenced this pull request Nov 12, 2020
This isn't the case as of the last commit.

[ci skip]
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. With my limited customremotes knowledge, these changes look good to me.

@mih
Copy link
Member Author

mih commented Nov 12, 2020

Thx @kyleam!

@mih
Copy link
Member Author

mih commented Nov 12, 2020

Last commit only changed a comment. Test runs were canceled, see those of the prev. commit.

@mih mih merged commit e4756f6 into datalad:master Nov 12, 2020
@mih mih deleted the rf-norunnercustomremote branch November 12, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants