Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor .travis.yml to deduplicate uses of Docker and use Mustache comments #7351

Merged
merged 7 commits into from Mar 11, 2019

Conversation

Projects
None yet
4 participants
@Eric-Arellano
Copy link
Contributor

commented Mar 10, 2019

Problem

#7235 introduced an initial deduplication of launching docker images, notably allowing the image name to be parametrized. While working on #7197, it became evident how frequently the same code was being repeated for calling docker run. The only thing that changes between these invocations is what goes inside sh -c "my bash command".

This duplication makes our Travis code harder to understand, along with the usual arguments for DRY.

Solution

Introduce a new docker_run_image mustache template that takes the parameters $docker_image_name and $docker_run_command.

Also changed:

  • rename launch_docker_image -> docker_build_image
  • remove script entry for &travis_docker_image, as it gets overridden every time by its caller so is unnecessary and confusing.
  • introduce Mustache comments to some of our templates. These don't get copied over into .travis.yml, which is good for reducing noise, while still allowing us to explain the concept in the template.

Result

CI should perform the same with the benefit of a shorter and better abstracted travis.yml.mustache.

Eric-Arellano added some commits Mar 10, 2019

Remove script entry for travis_docker_image
Now that we have the parameterized mustache template, we never end up even using this script entry. Every single use of this base anchor overrides script, so it simply adds noise and makes things more confusing.
Rename launch_docker_image to docker_build_image
Build is a more appropriate name than launch, because it mirrors docker. Also starting with `docker` will make `docker_run_image` more symmetrical.
Use comments in mustache templates
We want to explain some of our quirks, but its annoying for the comment to print to .travis.yml everytime it gets used. Instead, only put the comment once!
Fix Docker not evaluating external env var
THe bootstrap shards were failing to read their args (`-b` vs. `-2b`). This is because Docker expects a string with its exact arguments, and will not do any env var resolution from the external environment. Instead, all env vars must be valuated by the parent bash process and then the result passed to Docker.

This leads to slightly more duplication, but in some ways is more explicit and is an acceptable tradeoff.
@cosmicexplorer
Copy link
Contributor

left a comment

Considering that travis just laid off all its senior engineers I am 300% for decoupling more logic into separate files!

Show resolved Hide resolved build-support/travis/travis.yml.mustache
Show resolved Hide resolved build-support/travis/env_osx_with_pyenv.mustache Outdated

Eric-Arellano added some commits Mar 10, 2019

Provide comments explaining how to use env vars with templates
Danny pointed out it can be a bit confusing that we have two levels of templating: the mustache template and the env vars that it populates.

Add comments to the mustache templates stressing that it is expecting env vars and explaining what it expects these to be. This is kinda like docstring for mustache.

Also add comments in .travis.yml where we have a base image define a script, but then expect the callers of the anchor to provide env vars. Make this expectation clear.
Clarify OSX Pyenv template comment
It was a bit unclear why we need to set the env var and made it sound like there's an upcoming issue to resolve. Clarify this is intended and clarify how this fits in with everything else.
@illicitonion
Copy link
Contributor

left a comment

Works for me :) Thanks!

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Thanks all! Merging in order to land #7197. Happy to address any concerns Benjy or Stu that you may have in a followup PR.

@Eric-Arellano Eric-Arellano merged commit c835267 into pantsbuild:master Mar 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:dedupe-travis-docker branch Mar 11, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Mar 11, 2019

Remove bad leftover env var from pantsbuild#7351
pantsbuild#7351 should have deleted $BOOTSTRAP_ARGS for the Linux Build engine shards. This does its cleanup.
@benjyw
Copy link
Contributor

left a comment

LGTM

Eric-Arellano added a commit that referenced this pull request Mar 12, 2019

Create dedicated Dockerfile with Python 3.6 to reduce CI time (#7352)
### Problem
Every time we run a Docker shard, we spend about 3 minutes installing Python 3.6 and its dependencies, even if that shard never once uses Python 3. In particular, this slows down our `Linux Build Wheels UCS4` shard and our `Linux Build Engine Py2` for a collective cost of 6 minutes every CI run (i.e. total CI cost, not wall time).

Ideally, we would want Python 3 pre-installed on the base Centos6 image so that we never for any shard have to waste time installing Python 3 (see #7064). Until that lands, though, this is an additional unneeded burden.

### Solution
Create a new image `travis_ci_py36`.

Thanks to how we can now parametrize the docker image name in `travis.yml.mustache`, this brings no additional cognitive overload to our `.travis.yml`, which was the original reason we did not create a duplicate Dockerfile.

Also does a little cleanup from #7351 in removing a bad env var `$BOOTSTRAP_ARGS` for the linux build engine shards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.