Split docker-py tests from main image by adrielldagasuan · Pull Request #37939 · moby/moby

adrielldagasuan · 2018-09-30T14:26:43Z

Signed-off-by: Adriell Matthew Julius Dagasuan adrielldagasuan@gmail.com

- What I did
Addressed: #36415. Moved the docker-py tests from main docker-dev image into another docker-test image that will be used for testing

- How I did it
Removed the docker-py section from the main Dockerfile
Removed the installation of python-* packages from the main Dockerfile
Created Dockerfile.test as another image used for testing docker-py (and potentially other test scenarios)
Updated Makefile to build Dockerfile.test and use the image for testing

- How to verify it
Run make test-docker-py. It should build 2 images.
Verify that a docker-test image was created alongside the docker-dev image.
Verify that the docker-test image is larger than the docker-dev image, with the extra docker-py objects as well as the python-* packages.

REPOSITORY	TAG	IMAGE ID	SIZE
docker-test	36145-splitting-docker-py-section	859613acb285	2.04GB
docker-dev	36145-splitting-docker-py-section	89a6ffee61b3	1.94GB

Verify that the docker-dev image does not have the /docker-py directory by doing
docker run ... docker-dev:<tag> bash -c "ls /"

- Description for the changelog
Moved docker-py tests off the main docker-dev image into separate docker-test image

- A picture of a cute animal (not mandatory but encouraged)

adrielldagasuan · 2018-09-30T15:48:40Z

Is the janky build doing make all? Seems like it's able to get the correct version of the Dockerfile to build (w/o the installation of python-* packages and docker-py tests) but not the Makefile. Seems like it still runs the tests on the docker-dev image, rather than the docker-test image.

thaJeztah

Thanks for working on this! I left some thoughts inline, but only gave it a quick look. 😅

thaJeztah · 2018-10-01T14:39:28Z

Dockerfile.test

nit: missing a newline at the end

Also, the WORKDIR can be move more to the top (even before the first RUN), so that this step can be cached, even if the DOCKER_PY_COMMIT changes

thaJeztah · 2018-10-01T14:41:11Z

Dockerfile.test

I assume docker-pycred and yamllint can be installed independently from docker-py, so it's best to put them in their own RUN.

thaJeztah · 2018-10-01T14:43:28Z

Dockerfile.test

To optimise for caching, installation of docker-py should be in its own RUN. That way installation of the packages installed through apt-get ... will be cached, and only the cache for files installed for docker-py will be busted if DOCKER_PY_COMMIT changes

RUN git clone https://github.com/docker/docker-py.git /docker-py \ && cd /docker-py \ && git checkout -q $DOCKER_PY_COMMIT \ && pip install -r test-requirements.txt

thaJeztah · 2018-10-01T14:44:54Z

Dockerfile.test

Hm.. I'm personally never a fan of an empty default for FROM; ideally, it would have a sensible default, so that I can build the image successfully (docker build ..) without having to specify a --build-arg.

That will be a bit tricky in this case though

Now that buildkit will be more used overall, perhaps we could still include the docker-py image as a build-stage in the main Dockerfile; buildkit will skip that stage if it's not needed, and only build if the --target actually needs the build stage.

Doing so may make the FROM xyz easier to do.

@shin- @cpuguy83 @tonistiigi any suggestions?

thaJeztah · 2018-10-01T14:48:55Z

Looks like CI is failing on this;

14:47:12 Cloning into 'bundles/test-docker-py/docker-py'...
14:47:12 env: 'py.test': No such file or directory

adrielldagasuan · 2018-10-01T15:33:53Z

Thanks @thaJeztah for the quick review. I'll take your feedback and submit some changes. Some question on the janky build though. Is it just running make all on the branch, or is it doing something else? Based from the Jenkins logs, looks like it's still running the tests on the docker-dev image, which makes me think that the Makefile changes are not being considered.

adrielldagasuan · 2018-10-02T12:22:33Z

Based from TOOLS.md

The Docker project uses Jenkins as our continuous integration server. Each Pull Request to Docker is tested by running the equivalent of make all. We chose Jenkins because we can host it ourselves and we run Docker in Docker to test.

which leaves me to believe that it's not using the Makefile at all, but rather it has its instruction configured in Jenkins directly. For this change to pass the CI, I have to modify the Jenkins config to run the tests on the docker-test image, rather than the docker-dev image. But that might be hard to do considering that the Jenkins config applies to all PRs. Until we use Jenkinsfiles which will allow a different pipeline per build, I think this will be stuck for a while.

That said, it might be worth doing that change first (mentioned in #36416), to allow for changes like these to work.

I can push the changes to address the feedback from but at the same time still waiting for comments regarding including the docker-py stage in the main docker-dev image (but having the option to skip it).

Thoughts @thaJeztah @vdemeester ?

GordonTheTurtle · 2018-10-07T15:25:15Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "36145-splitting-docker-py-section" git@github.com:adrielldagasuan/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

codecov · 2018-10-07T15:25:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b5ed4eb). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #37939   +/-   ##
=========================================
  Coverage          ?    36.1%           
=========================================
  Files             ?      610           
  Lines             ?    45126           
  Branches          ?        0           
=========================================
  Hits              ?    16293           
  Misses            ?    26594           
  Partials          ?     2239

Signed-off-by: Adriell Matthew Julius Dagasuan <adrielldagasuan@gmail.com>

adrielldagasuan · 2018-10-07T15:33:56Z

Updated the code based on the feedback. But I don't think this will pass the CI since the CI has to be updated to pass --target=test. The final image wouldn't have the docker-py content. So if the CI has to run the docker-py tests, it has to either to pass the target flag.

olljanat · 2018-12-22T07:16:37Z

@adrielldagasuan can you fix those failing tests?

thaJeztah · 2019-07-18T19:54:15Z

obsoleted by #39068

thanks @adrielldagasuan for working on this though!

GordonTheTurtle added the status/0-triage label Sep 30, 2018

thaJeztah added status/2-code-review area/testing and removed status/0-triage labels Oct 1, 2018

thaJeztah requested changes Oct 1, 2018

View reviewed changes

adrielldagasuan force-pushed the 36145-splitting-docker-py-section branch from fb67373 to 07fd590 Compare October 7, 2018 15:25

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 7, 2018

Split docker-py tests from main image

78c655e

Signed-off-by: Adriell Matthew Julius Dagasuan <adrielldagasuan@gmail.com>

adrielldagasuan force-pushed the 36145-splitting-docker-py-section branch from 07fd590 to 78c655e Compare October 7, 2018 15:27

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 7, 2018

GordonTheTurtle assigned runcom Oct 15, 2018

derek bot added the rebuild/* label Dec 21, 2018

GordonTheTurtle removed the rebuild/* label Dec 21, 2018

derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018

thaJeztah mentioned this pull request Apr 16, 2019

Bump docker-py to 4.0.2, and run tests from upstream repository #39068

Merged

tiborvass closed this in #39068 Jul 18, 2019

Conversation

adrielldagasuan commented Sep 30, 2018

Uh oh!

adrielldagasuan commented Sep 30, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 1, 2018

Uh oh!

adrielldagasuan commented Oct 1, 2018

Uh oh!

adrielldagasuan commented Oct 2, 2018

Uh oh!

GordonTheTurtle commented Oct 7, 2018

Uh oh!

codecov bot commented Oct 7, 2018

Codecov Report

Uh oh!

adrielldagasuan commented Oct 7, 2018

Uh oh!

olljanat commented Dec 22, 2018

Uh oh!

thaJeztah commented Jul 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants