bug 1352232: Remove Ansible from py27 build #4239

jwhitlock · 2017-05-16T00:59:34Z

Don't use Ansible to install MySQL and ElasticSearch. Instead:

Use TravisCI's MySQL, without the custom collation or migrations. Docker continues to test w/ utf8_distinct_ci and migrations.
Install ElasticSearch as suggested by TravisCI docs
Install node packages needed by Django pipeline, so that make build-static will work for tests.
Rewrite scripts/travis_install to use env variables like INSTALL_ELASTICSEARCH=1, instead of targeting on TOXENV, to make it easier to combine and customize.

All of these changes makes the py27 target faster to build (8 minutes vs 11 min 30 seconds, 30% faster), and easier to customize. I've added the first customization for bug 1352232, where tests fail if PYTHONHASHSEED is random, which affects our readiness for Python 3.

@jgmize this might be an easy 👍, requesting your review if you can get to it by next Monday.

codecov-io · 2017-05-16T15:40:00Z

Codecov Report

Merging #4239 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4239      +/-   ##
==========================================
+ Coverage   86.57%   86.58%   +0.01%     
==========================================
  Files         148      150       +2     
  Lines        9048     9065      +17     
  Branches     1199     1198       -1     
==========================================
+ Hits         7833     7849      +16     
- Misses        988      989       +1     
  Partials      227      227

Impacted Files	Coverage Δ
kuma/core/utils.py	`55.25% <0%> (-0.39%)`	⬇️
kuma/users/views.py	`96.42% <0%> (-0.33%)`	⬇️
kuma/wiki/models.py	`89.27% <0%> (-0.23%)`	⬇️
kuma/urls.py	`80.76% <0%> (ø)`	⬆️
kuma/health/urls.py	`100% <0%> (ø)`
kuma/health/views.py	`100% <0%> (ø)`
kuma/settings/common.py	`94.28% <0%> (+0.95%)`	⬆️
kuma/wiki/views/list.py	`70.7% <0%> (+1.01%)`	⬆️

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 b44af16...c0e6ec2. Read the comment docs.

Switch to TravisCI-provided MySQL without custom collation. This requires running tests with --no-migrations. The Docker build continues to use MySQL with the custom collation utf8_distinct_ci and migraations. Switch to the TravicCI-recommended method for installing and running ElasticSearch. Update caching so that node installed and downloaded (ElasticSearch and docker-compose) are fast when the version doesn't change. Move configuration environment variables to global, and use additional variables to control what gets installed (rather than TOXENV): * CREATE_DB=dbname - Runs MySQL database creation for Django tests * INSTALL_DOCKER_COMPOSE - Replaces docker_compose with the specific version. * INSTALL_ELASTICSEARCH=1 - Downloads and runs ElasticSearch, and ensures it is ready before starting tests * INSTALL_PIPELINE=1 - Installs node packages used by django-pipeline, so that "make build-static" will run successfully

Drop PYTHONPATH, CFLAGS, and DEBIAN_FRONTED from the environment overrides.

Add a build (allowed to fail) that tests Kuma with a random PYTHONHASHSEED.

safwanrahman · 2017-05-17T19:43:26Z

scripts/travis-install

-    sudo ln -s /usr/lib/`uname -i`-linux-gnu/libfreetype.so /usr/lib
-    sudo ln -s /usr/lib/`uname -i`-linux-gnu/libz.so /usr/lib
+    mkdir -p downloads
+    wget -q -O downloads/elasticsearch-$ES_VERSION.tar.gz $ES_DOWNLOAD_URL


Can you build the url here?
Rather than passing the url in environment parameters

No, you can't. ElasticSearch keeps changing their downloads domain. Here's a sample:

Version URL

2.2.0 https://download.elasticsearch.org/elasticsearch/release/org/elasticsearch/distribution/tar/elasticsearch/2.2.0/elasticsearch-2.2.0.tar.gz

1.7.6 https://download.elastic.co/elasticsearch/elasticsearch/elasticsearch-1.7.6.tar.gz

5.3.0 https://artifacts.elastic.co/downloads/elasticsearch/elasticsearch-5.3.0.tar.gz

jgmize · 2017-05-17T19:50:13Z

.travis.yml

+    # Wait for ElasticSearch to be ready
+    - if [ ${INSTALL_ELASTICSEARCH:-0} -ne 0 ]; then wget -q --waitretry=1 --retry-connrefused -T 10 -O - http://127.0.0.1:9200; fi;
+script:
+    if [[ "${PYTHONHASHSEED:-}" == "0" ]]; then


${PYTHONHASHSEED:-} seems redundant, I think the simpler ${PYTHONHASHSEED} would be equivalent.

safwanrahman · 2017-05-17T20:12:25Z

scripts/travis-install

+            https://github.com/docker/compose/releases/download/${DOCKER_COMPOSE_VERSION}/docker-compose-`uname -s`-`uname -m`
+    fi
+    chmod +x $DOCKER_COMPOSE_FILE
+    sudo cp $DOCKER_COMPOSE_FILE /usr/local/bin/docker-compose


Can we cache this path and check if the version matches with the provided one?

INSTALLED_DOCER_COMPOSE_VERSION=$(docker-compose -v | grep -o '[0-9]\?[0-9].[0-9]\?[0-9].\?[0-9]') if ! [ INSTALLED_DOCER_COMPOSE_VERSION == DOCKER_COMPOSE_VERSION]; then # Download and install fi

No, that will never ever happen. The VM is reset each time, so the version will be 1.8.1 until TravisCI updates their VM image. When they do that, the new default may be later than 1.9.0, and we can stop installing our own docker-compose. Until then, the download folder is cached, so we don't download the file unless we change DOCKER_COMPOSE_VERSION.

@jwhitlock We can add /usr/local/bin/docker-compose to cache config to keep the docker-compose

You can cache directories but not files. I think it would be a mistake to cache /usr/local/bin, so I think we're OK caching the file in downloads and copying it each time.

the docker-compose file is already in the downloads dir, which is in the cache due to https://github.com/mozilla/kuma/pull/4239/files#diff-354f30a63fb0907d4ad57269548329e3R9

Use simple bash tests in .travis.yml Use set -u in scripts/travis-install, instead of checking that the right environment variables are set.

safwanrahman · 2017-05-18T23:40:24Z

.travis.yml

 install:
-    - pip install -r requirements/travis.txt
-    - if [[ $TOXENV == 'py27' ]]; then ansible-playbook -vvv --tags=mysql,pipeline --connection=local --inventory-file=provisioning/inventory provisioning/travis.yml; fi
    - nvm install 6


Can we include all the installs in scripts/travis-install?
We dont need to install pip and other thing in docker testing environment. So it will faster the build

This might be a good follow-on PR. requirements/travis.txt includes coveralls, which is called at the end of each test, so you'd need to ensure coveralls is not called for the targets that generate coverage data. Which maybe should include the docker target, but currently doesn't. But it could be skipped for the linter tasks.

That's also a lot of work to save at most 10s per build.

safwanrahman · 2017-05-18T23:53:55Z

tox.ini

-    PYTHONHASHSEED = 0
-passenv=DJANGO_SETTINGS_MODULE DEBIAN_FRONTEND CFLAGS
+    make compilejsi18n collectstatic clean
+    py.test --no-migrations --cov=kuma kuma


If new migration is added, will it fail?

Not here, since we aren't running them due to --no-migrations.

safwanrahman · 2017-05-18T23:58:57Z

.travis.yml

-        - TOXENV=docker DOCKER_COMPOSE_VERSION=1.9.0 UID=0
+        - TOXENV=locales
+          INSTALL_DOCKER_COMPOSE=1
+        - TOXENV=docker


Keeping it in single line seems more readable to me TOXENV=docker INSTALL_DOCKER_COMPOSE=1 UID=0

I prefer the new multi-line approach.

jgmize

Nice work @jwhitlock

jgmize · 2017-05-19T19:25:54Z

Thanks for your thorough review and questions @safwanrahman. I think they've all been addressed and this is good to merge.

jwhitlock added the not ready label May 16, 2017

jwhitlock force-pushed the no-ansible-1352232 branch 2 times, most recently from 31e8376 to ae5f594 Compare May 17, 2017 15:23

jwhitlock added 2 commits May 17, 2017 11:08

bug 1352232: Less Travis-specific environment

e6d8b66

Drop PYTHONPATH, CFLAGS, and DEBIAN_FRONTED from the environment overrides.

jwhitlock force-pushed the no-ansible-1352232 branch from ae5f594 to e6d8b66 Compare May 17, 2017 16:14

bug 1127798: Add build with random PYTHONHASHSEED

dbb0d94

Add a build (allowed to fail) that tests Kuma with a random PYTHONHASHSEED.

jwhitlock changed the title ~~bug 1352232 (WIP): Remove Ansible from py27 build~~ bug 1352232: Remove Ansible from py27 build May 17, 2017

jwhitlock requested a review from jgmize May 17, 2017 17:23

jwhitlock removed the not ready label May 17, 2017

safwanrahman reviewed May 17, 2017

View reviewed changes

jgmize reviewed May 17, 2017

View reviewed changes

safwanrahman reviewed May 17, 2017

View reviewed changes

bug 1352232: Use appropriate bash

c0e6ec2

Use simple bash tests in .travis.yml Use set -u in scripts/travis-install, instead of checking that the right environment variables are set.

safwanrahman reviewed May 18, 2017

View reviewed changes

jgmize approved these changes May 19, 2017

View reviewed changes

jgmize merged commit 46e9ab9 into master May 19, 2017

jgmize deleted the no-ansible-1352232 branch May 19, 2017 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug 1352232: Remove Ansible from py27 build #4239

bug 1352232: Remove Ansible from py27 build #4239

jwhitlock commented May 16, 2017 •

edited

codecov-io commented May 16, 2017 •

edited

safwanrahman May 17, 2017

jwhitlock May 17, 2017

jgmize May 17, 2017

safwanrahman May 17, 2017

jwhitlock May 17, 2017

safwanrahman May 18, 2017 •

edited

jwhitlock May 19, 2017

jgmize May 19, 2017

safwanrahman May 18, 2017

jwhitlock May 19, 2017

safwanrahman May 18, 2017

jgmize May 19, 2017

safwanrahman May 18, 2017

jgmize May 19, 2017

jgmize left a comment

jgmize commented May 19, 2017

Version	URL
2.2.0	https://download.elasticsearch.org/elasticsearch/release/org/elasticsearch/distribution/tar/elasticsearch/2.2.0/elasticsearch-2.2.0.tar.gz
1.7.6	https://download.elastic.co/elasticsearch/elasticsearch/elasticsearch-1.7.6.tar.gz
5.3.0	https://artifacts.elastic.co/downloads/elasticsearch/elasticsearch-5.3.0.tar.gz

bug 1352232: Remove Ansible from py27 build #4239

bug 1352232: Remove Ansible from py27 build #4239

Conversation

jwhitlock commented May 16, 2017 • edited

codecov-io commented May 16, 2017 • edited

Codecov Report

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

safwanrahman May 18, 2017 • edited

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

jgmize left a comment

Choose a reason for hiding this comment

jgmize commented May 19, 2017

jwhitlock commented May 16, 2017 •

edited

codecov-io commented May 16, 2017 •

edited

safwanrahman May 18, 2017 •

edited