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

BF: docker adapter: Don't couple 'run -i' with stdin.isatty #142

Merged
merged 1 commit into from
Dec 3, 2020

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Nov 17, 2020

24af27f (BF: docker adapter: Drop -it flags when stdin is not a tty,
2019-11-11) dropped both --interactive and --tty from the 'docker run'
call if stdin.isatty() reports false, avoiding an "the input device is
not a TTY" failure in this case.

However, dropping --interactive shouldn't be necessary, and doing so
prevents the command from receiving stdin.


I noticed this issue when looking into gh-123.

Before this PR:

$ echo foo | md5sum
d3b07384d113edec49eaa6238ad5ff00  -
$ echo foo | datalad containers-run -n alp md5sum
[INFO] Making sure inputs are available (this may take some time)
[INFO] == Command start (output follows) =====
d41d8cd98f00b204e9800998ecf8427e  -
[INFO] == Command exit (modification check follows) =====
[INFO] Total: starting
[INFO]
[INFO] Total: processed result for /tmp/po-TYjIvuy
[INFO] Total: done
action summary:
  get (notneeded: 1)
  save (notneeded: 1)

After:

$ echo foo | datalad containers-run -n alp md5sum
[INFO] Making sure inputs are available (this may take some time)
[INFO] == Command start (output follows) =====
d3b07384d113edec49eaa6238ad5ff00  -
[INFO] == Command exit (modification check follows) =====
[INFO] Total: starting
[INFO]
[INFO] Total: processed result for /tmp/po-TYjIvuy
[INFO] Total: done
action summary:
  get (notneeded: 1)
  save (notneeded: 1)

24af27f (BF: docker adapter: Drop -it flags when stdin is not a tty,
2019-11-11) dropped both --interactive and --tty from the 'docker run'
call if stdin.isatty() reports false, avoiding an "the input device is
not a TTY" failure in this case.

However, dropping --interactive shouldn't be necessary, and doing so
prevents the command from receiving stdin.
@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #142 (0624181) into master (d958d80) will increase coverage by 0.04%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   86.50%   86.54%   +0.04%     
==========================================
  Files          17       17              
  Lines         926      929       +3     
==========================================
+ Hits          801      804       +3     
  Misses        125      125              
Impacted Files Coverage Δ
datalad_container/adapters/docker.py 35.95% <0.00%> (ø)
datalad_container/adapters/tests/test_docker.py 93.54% <100.00%> (+0.32%) ⬆️

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 399eaaf...0624181. Read the comment docs.

@kyleam
Copy link
Contributor Author

kyleam commented Dec 1, 2020

I'll plan to wait another day or two for objections and then merge.

@kyleam kyleam merged commit 94f9ad1 into datalad:master Dec 3, 2020
@kyleam kyleam deleted the docker-adapter-stdin branch December 3, 2020 17:54
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.

1 participant