ReproNim / reproman

ReproMan (AKA NICEMAN, AKA ReproNim TRD3)
https://reproman.readthedocs.io
Other
24 stars 14 forks source link

MNT: Unpin docker-py #481

Closed kyleam closed 4 years ago

kyleam commented 4 years ago
The docker module hasn't been released under the name "docker-py"
since 2016 [0] and has seen many releases under the name "docker" [1].
So we've effectively been pinning the release to version 1.10.6 even
though the current version is 4.1.0.

To update to latest version, it looks like we can get by with
substituting APIClient for Client.  Set the new floor to version
2.3.0, the release that introduced APIClient.

[0]: https://pypi.org/project/docker-py
[1]: https://pypi.org/project/docker

Closes #387.


Light local testing suggests this might work, but let's see what the test run says. Also, before merging, we should make sure dockerpty is hooked up correctly (see gh-387).

codecov[bot] commented 4 years ago

Codecov Report

Merging #481 into master will decrease coverage by 0.17%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
- Coverage   89.54%   89.36%   -0.18%     
==========================================
  Files         148      148              
  Lines       12132    12132              
==========================================
- Hits        10863    10842      -21     
- Misses       1269     1290      +21
Impacted Files Coverage Δ
reproman/interface/tests/test_install.py 96.29% <100%> (ø) :arrow_up:
reproman/interface/tests/test_login.py 100% <100%> (ø) :arrow_up:
reproman/resource/docker_container.py 94% <100%> (ø) :arrow_up:
reproman/interface/tests/test_ls.py 100% <100%> (ø) :arrow_up:
reproman/resource/tests/test_docker_container.py 100% <100%> (ø) :arrow_up:
reproman/interface/tests/test_create.py 100% <100%> (ø) :arrow_up:
reproman/resource/tests/test_session.py 99.54% <100%> (ø) :arrow_up:
reproman/distributions/tests/test_docker.py 100% <100%> (ø) :arrow_up:
reproman/interface/tests/test_delete.py 100% <100%> (ø) :arrow_up:
reproman/distributions/tests/test_venv.py 87.5% <0%> (-10.58%) :arrow_down:
... and 7 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 2e39954...8ab2d0e. Read the comment docs.

kyleam commented 4 years ago

I said:

To update to latest version, it looks like we can get by with substituting APIClient for Client.

Not quite. We also need to update for get_archives new return value.

range-diff ``` 1: 5bfa0116c ! 1: 8ab2d0e17 MNT: Unpin docker-py @@ Commit message So we've effectively been pinning the release to version 1.10.6 even though the current version is 4.1.0. - To update to latest version, it looks like we can get by with - substituting APIClient for Client. Set the new floor to version - 2.3.0, the release that introduced APIClient. + To update to latest version, we need to substitute APIClient for + Client (renamed in v2.3.0). We also need to teach DockerSession.get() + about the new return value of APIClient.get_archive(): as of v3.0.0, + the content is returned as a generator of chunks rather than a byte + string. + + Specify v3.0.0 as the new docker-py floor in setup.py Closes #387. @@ reproman/resource/docker_container.py: def connect(self): containers = [] for container in self._client.containers(all=True): +@@ reproman/resource/docker_container.py: def get(self, src_path, dest_path=None, uid=-1, gid=-1): + dest_path = self._prepare_dest_path(src_path, dest_path) + dest_dir = os.path.dirname(dest_path) + stream, stat = self.client.get_archive(self.container, src_path) +- tarball = tarfile.open(fileobj=io.BytesIO(stream.read())) ++ # get_archive() returns a generator with the content (in 2 MB chunks by ++ # default). Consider exposing those chunks as a stream rather than ++ # joining them. ++ tarball = tarfile.open(fileobj=io.BytesIO(b"".join(stream))) + tarball.extractall(path=dest_dir) + os.rename(os.path.join(dest_dir, src_basename), dest_path) + ## reproman/resource/tests/test_docker_container.py ## @@ @@ setup.py: def findsome(subdir, extensions): ], 'docker': [ - 'docker-py>=0.3.2', -+ 'docker>=2.3.0', ++ 'docker>=3.0.0', 'dockerpty', ], 'aws': [ ```
kyleam commented 4 years ago

Also, before merging, we should make sure dockerpty is hooked up correctly (see gh-387).

Tested reproman login locally. Worked fine.

kyleam commented 4 years ago

Thanks for taking a look.