Add new Makefile target to run it tests inside Docker #487

dliappis · 2018-04-30T09:40:04Z

it tests and tox relying on a number of Python 3 versions via
pyenv are a good candidate for running through Docker, as setting up
pyenv and the needed Python versions (as well as required
JAVA_HOME/RUNTIME_JAVA_HOME) can be fiddly.

This is an easier task than #484
as it doesn't rely on passing ssh credentials via the ssh-agent. Only
issue is that on operating systems where Docker runs as a VM (macOS,
Windows) the default configured memory will not to be increased in
order to run the entirety of the integration tests, as Rally uses the
4GB car for Elasticsearch (on top of additional tasks). However, this
target is especially suitable for running integration tests inside a
powerful server/workstation Linux environment without having to worry
about configuring all the dependencies.

`it` tests and `tox` relying on a number of Python 3 versions via pyenv are a good candidate for running through Docker, as setting up pyenv and the needed Python versions (as well as required JAVA_HOME/RUNTIME_JAVA_HOME) can be fiddly. This is an easier task than elastic#484 as it doesn't rely on passing ssh credentials via the ssh-agent. Only issue is that on operating systems where Docker runs as a VM (macOS, Windows) the default configured memory will not to be increased in order to run the entirety of the integration tests, as Rally uses the 4GB car for Elasticsearch (on top of additional tasks). However, this target is especially suitable for running integration tests inside a powerful server/workstation Linux environment without having to worry about configuring all the dependencies.

Under certain conditions (and Makefile versions) it's not sufficient to just export UID for Docker/docker-compose to consume (Ubuntu) whereas in other cases (RedHat) setting UID makes the shell complain about setting a read-only variable. Be more resilient about ensuring UID is present as an environment variable when spawning (Docker) processes through the Makefile.

This reverts commit f741b32.

danielmitterdorfer

Thanks for the PR. I left some comments.

danielmitterdorfer · 2018-05-02T06:33:41Z

Dockerfile

+    openjdk-8-jdk
+
+RUN pip3 install tox virtualenvwrapper twine sphinx sphinx_rtd_theme wheel
+RUN pip3 install --pre github3.py


I think this is not needed to run integration tests?

No I thought we might keep it in case we give another try at #484 in the future with the same image. But I guess it makes sense to keep it clean for now.

danielmitterdorfer · 2018-05-02T06:36:48Z

docker-compose.yml

+    volumes:
+      - $HOME/.gitconfig:/home/${USER}/.gitconfig
+      - $HOME/.github:/home/${USER}/.github
+      - $HOME/.gnupg:/home/${USER}/.gnupg


Is it required to mount these directories?

No, this was left over with the same idea as before, i.e. maybe we could use this for another attempt at #484 in the future. I'll remove it for now.

danielmitterdorfer · 2018-05-02T06:37:37Z

Makefile

@@ -34,4 +43,11 @@ release-checks:
 release: release-checks clean docs it
 	./release.sh $(release_version) $(next_version)

-.PHONY: clean docs test it it34 it35 it36 benchmark coverage release release-checks
+docker-it: nondocs-clean python-caches-clean
+	@if ! export | grep UID; then export UID=$(shell id -u); fi ; \


I am not familiar with this syntax. Why is there an @ in the first line?

@ causes make to suppress echoing: https://www.gnu.org/software/make/manual/make.html#Echoing

less junk on the output esp. with if's that can confuse the user.

Didn't know about that. I also saw now that this is actually just one long line separated by \, i.e. all output from this target will get suppressed.

Just a small correction not all output will get suppressed (e.g. the output generated from the commands themselves e.g. make it output will get displayed as expected).

e.g. if you have a Makefile target called:

testme: @ls

this will display the ls output normally but not the ls command itself, which would be the case without @.

Yes, I should have said "echoing" instead of "output". Thanks for the explanation.

danielmitterdorfer · 2018-05-02T06:38:22Z

Makefile

@@ -1,7 +1,16 @@
-clean:
+clean: nondocs-clean docs-clean


I wonder whether clean should clean everything (including Python caches).

I am ambivalent about this one because it you don't use docker it will introduce some slowness every time you run make docs or make it as I've noticed some downloads happening. At the same time it ensures consistency.

Ok, it does not make sense to clean Python caches for make docs. However, I think for make it would make sense?

Yes it would make sense for make it. I think we should add it to the individual it34/35/36 targets too?

Yes, please.

danielmitterdorfer · 2018-05-02T06:40:34Z

Makefile

 	cd docs && $(MAKE) clean

+# Avoid conflicts between .pyc/pycache related files created by local Python interpreters and other interpreters in Docker
+python-caches-clean:


If I type make python-caches-clean locally (on MacOS), I get:

(.venv) daniel@io:Projects/rally ‹pr/487›$ make python-caches-clean find ./ -name "__pycache__" -exec rm -rf -- \{\} \; find: .//.venv/lib/python3.6/__pycache__: No such file or directory [...] find: .//tests/utils/__pycache__: No such file or directory make: [python-caches-clean] Error 1 (ignored) find ./ -name ".pyc" -exec rm -rf -- \{\} \;

As discussed, this is harmless and should always return exit 0 due to the - prefix in the command.

Consecutive slashes will get collapsed as per the Single Unix Specification.

Seems that using . instead of ./ makes the output look saner on macOS so I'll push this plus an @ to suppress ~~output~~ echoing altogether.

Thanks. Makes sense.

dliappis · 2018-05-02T07:54:35Z

@danielmitterdorfer I've addressed your comments, could have another pass at the PR?

danielmitterdorfer

I left two more comments.

danielmitterdorfer · 2018-05-02T07:55:42Z

Makefile

 	tox -e py34

-it35:
+it35: python-caches-clean
 	tox -e py35

 it36:


Should also need python-caches-clean?

Thanks forgot this one! Addressed it now.

danielmitterdorfer · 2018-05-02T07:56:42Z

docker-compose.yml

@@ -8,9 +8,6 @@ services:
        - NEW_USER=${USER}
    container_name: rally-tests
    volumes:
-      - $HOME/.gitconfig:/home/${USER}/.gitconfig
-      - $HOME/.github:/home/${USER}/.github
-      - $HOME/.gnupg:/home/${USER}/.gnupg
      - ${PWD}/.:/home/${USER}/rally


I missed this in my first pass. Doesn't this assume that the user's Rally project directory is available in ~/rally?

Correct. The CWD is changed here: https://github.com/dliappis/rally/blob/79c6ab8fad02f6509bb510c116c8d3de8b6b0cc0/docker-compose.yml#L14 .

The reason it's done this way is that any dot files created under /home/$USER inside the docker container shouldn't interfere with the dot files (e.g. ~/.rally) of the host system.

Ah, you're right. This is the path in the container, not on the host.

dliappis · 2018-05-02T08:01:40Z

@dm can you have another look now?

danielmitterdorfer · 2018-05-02T08:10:41Z

Thanks. LGTM! :)

dliappis requested a review from danielmitterdorfer April 30, 2018 09:40

dliappis added :Usability Makes Rally easier to use idea labels Apr 30, 2018

dliappis added this to the 0.11.0 milestone Apr 30, 2018

dliappis added 2 commits April 30, 2018 13:20

Revert "Be more resilient across distributions with UID"

0d750b0

This reverts commit f741b32.

danielmitterdorfer requested changes May 2, 2018

View reviewed changes

danielmitterdorfer added enhancement Improves the status quo and removed idea labels May 2, 2018

dliappis added 3 commits May 2, 2018 10:00

Addressing PR comment

9f7b83b

Addressing PR comments

f5e6bc8

Addressing PR comments

79c6ab8

danielmitterdorfer reviewed May 2, 2018

View reviewed changes

Addressing PR comment

25c7b66

danielmitterdorfer approved these changes May 2, 2018

View reviewed changes

dliappis merged commit 4805941 into elastic:master May 2, 2018

dliappis deleted the add-target-to-run-it-tests-in-docker branch May 2, 2018 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Makefile target to run it tests inside Docker #487

Add new Makefile target to run it tests inside Docker #487

dliappis commented Apr 30, 2018

danielmitterdorfer left a comment

danielmitterdorfer May 2, 2018

dliappis May 2, 2018

danielmitterdorfer May 2, 2018

dliappis May 2, 2018

danielmitterdorfer May 2, 2018

dliappis May 2, 2018

danielmitterdorfer May 2, 2018

dliappis May 2, 2018

danielmitterdorfer May 2, 2018

danielmitterdorfer May 2, 2018

dliappis May 2, 2018

danielmitterdorfer May 2, 2018

dliappis May 2, 2018

danielmitterdorfer May 2, 2018

danielmitterdorfer May 2, 2018

dliappis May 2, 2018 •

edited

Loading

danielmitterdorfer May 2, 2018

dliappis commented May 2, 2018

danielmitterdorfer left a comment

danielmitterdorfer May 2, 2018

dliappis May 2, 2018

danielmitterdorfer May 2, 2018

dliappis May 2, 2018

danielmitterdorfer May 2, 2018

dliappis commented May 2, 2018

danielmitterdorfer commented May 2, 2018

Add new Makefile target to run it tests inside Docker #487

Add new Makefile target to run it tests inside Docker #487

Conversation

dliappis commented Apr 30, 2018

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis commented May 2, 2018

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis commented May 2, 2018

danielmitterdorfer commented May 2, 2018

dliappis May 2, 2018 •

edited

Loading