ros-tooling / cross_compile

A tool to build ROS and ROS2 workspaces for various targets
Apache License 2.0
188 stars 60 forks source link

Actually stream docker run output live and add test for it #167

Closed emersonknapp closed 4 years ago

emersonknapp commented 4 years ago

Closes https://github.com/ros-tooling/cross_compile/issues/152

Ensure that docker run logs are streamed live as they run. I had earlier used an argument (run(stream=True)) that was added in a newer version of docker-py than the one we specify.

We can't remove the container automatically, then, because we have to query its exit code in order to raise an exception on a failure.

Signed-off-by: Emerson Knapp emerson.b.knapp@gmail.com

codecov[bot] commented 4 years ago

Codecov Report

Merging #167 into master will increase coverage by 0.19%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   93.66%   93.85%   +0.19%     
==========================================
  Files           7        7              
  Lines         221      228       +7     
==========================================
+ Hits          207      214       +7     
  Misses         14       14
Flag Coverage Δ
#unittests 93.85% <100%> (+0.19%) :arrow_up:
Impacted Files Coverage Δ
ros_cross_compile/docker_client.py 100% <100%> (ø) :arrow_up:

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 a7e1a2a...d946d54. Read the comment docs.

thomas-moulard commented 4 years ago

If you need a more recent version of pydocker, don't you need to bump the requirements in setup.py?

emersonknapp commented 4 years ago

If you need a more recent version of pydocker, don't you need to bump the requirements in setup.py?

I had previously used an argument that only exists in a newer version we aren't installing (I was looking at the "latest" readthedocs whoops) - however this function silently accepts unknown keyword arguments so there was no obvious indication that it wasn't working. I've changed the code to match the version that we are already installing, since we decided against moving to the newer version.