Skip to content

docker-py: fix linting, generate junit.xml, save bundles, and various improvements#39716

Merged
thaJeztah merged 6 commits intomoby:masterfrom
thaJeztah:docker_py_linting_and_improvements
Aug 19, 2019
Merged

docker-py: fix linting, generate junit.xml, save bundles, and various improvements#39716
thaJeztah merged 6 commits intomoby:masterfrom
thaJeztah:docker_py_linting_and_improvements

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 10, 2019

Addresses parts of #39675

built on top of #39714 to get CI green

  • fix linting issues reported by shellcheck
  • run without tty to disable color output
  • use --mount for bind-mounting docker.sock
  • output junit.xml for test-results and send them to Jenkins
  • build dynamic binary for docker-py, to match makefile
    This also makes sure that we can test all functionality of the daemon, because some features are not available on static binaries.
  • save docker-py artifacts

@thaJeztah
Copy link
Member Author

I opened a PR in the upstream docker-py respository to update the version of pytest that's used, because the junit.xml generated by the current version may be rejected by Jenkins; docker/docker-py#2409

@thaJeztah
Copy link
Member Author

Removing the --durations=10 commit, because the image is configured to print a short summary at the end (which apparently suppresses that info)

@thaJeztah thaJeztah force-pushed the docker_py_linting_and_improvements branch 2 times, most recently from 114a71d to 553e4de Compare August 10, 2019 21:32
@thaJeztah thaJeztah force-pushed the docker_py_linting_and_improvements branch 6 times, most recently from 36cb872 to fdfbfbb Compare August 10, 2019 23:57
@thaJeztah
Copy link
Member Author

Whoop; it works

Screenshot 2019-08-11 at 02 09 38

@thaJeztah thaJeztah force-pushed the docker_py_linting_and_improvements branch 3 times, most recently from 6d44cb4 to eca931c Compare August 11, 2019 13:04
Copy link
Contributor

Choose a reason for hiding this comment

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

??

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

this means you can't ^C

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like that still works;

INFO: Starting docker-py tests...
============================= test session starts ==============================
platform linux -- Python 3.6.9, pytest-4.1.0, py-1.8.0, pluggy-0.12.0
rootdir: /src, inifile: pytest.ini
plugins: cov-2.6.1, timeout-1.3.3
collected 385 items / 5 deselected

tests/integration/api_build_test.py ....^Cmake: *** [test-docker-py] Error 130

Copy link
Contributor

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

- SC2006: use $(...) notation instead of legacy backticked `...`
- SC2086: double quote to prevent globbing and word splitting

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This also makes sure that we can test all functionality of the
daemon, because some features are not available on static binaries.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the docker_py_linting_and_improvements branch from eca931c to 8b6da9d Compare August 15, 2019 00:52
@thaJeztah
Copy link
Member Author

Rebased, and removed the -v (verbose) from the tar line

@thaJeztah
Copy link
Member Author

All green; merging this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants