pantheon-systems / docker-build-tools-ci

Dockerfile source for docker image pantheon-public/build-tools-ci on quay.io
34 stars 39 forks source link

Wasting Pantheon Resources #40

Closed ccjjmartin closed 5 years ago

ccjjmartin commented 5 years ago

FYI, this line and the few lines following it are most likely wasting a lot of resources at Pantheon:

https://github.com/pantheon-systems/docker-build-tools-ci/blob/1.x/scripts/set-environment#L23

The reason for this being that a normal workflow involves developers pushing code up to GitHub which creates a new branch on GitHub and triggers the creation of a multidev with a name of ci-?? this process is most likely running in the background while the developer is actually creating a PR. Once the PR is created a second multidev is created with the name pr-?? so now two multidevs show the exact same code.

I would suggest only using one approach to naming multidevs on Pantheon, ci- or pr- but not both in the same script. We actually have opted for not using either of the ones provided (based on CI_BUILD_NUM or CIRCLE_PULL_REQUEST, instead we are creating multidevs based on branch name CIRCLE_BRANCH.

Recently I have been editing my config.yml to excluding pulling from this file externally and using an overwritten one in the .circleci/scripts/pantheon directory.

We have two different solutions right now:

The first solution uses the branch name and trims / adjusts it to work on Pantheon, this is directly in config.yml

echo "export ENV_NAME=$(echo $CIRCLE_BRANCH | tr '[:upper:]' '[:lower:]' | sed 's/.*\///' | sed 's/[^0-9a-z-]//g' | cut -c -11 | sed 's/-$//')" >> $BASH_ENV

The second solution uses a function to process the name given into a Pantheon compatible name:

pantheon_env_from_branch () {
  local branch="$1"
  # Rules to follow are:
  # Multidev branch names must be all lowercase, be less than 11 characters, but may contain a dash (-).
  # Environments cannot be created with the following reserved names: master, settings, team, support, multidev, debug, files, tags, and billing.
  # Environment names must be valid urls and cannot end with -.
  if [[ -z "$TERMINUS_ENV_PRUNE_PATTERN" ]]; then
    TERMINUS_ENV_PRUNE_PATTERN="^[^-]*-";
  fi
  local new_env="ci-$(echo $1 | tr '[:upper:]' '[:lower:]' | sed -e "s/$TERMINUS_ENV_PRUNE_PATTERN//g" | tr -cd '[[a-zA-Z0-9]]-' | sed -r 's/(.{7})-/\1/g' | cut -c1-8)"
  echo $new_env
}

DEFAULT_ENV=$(pantheon_env_from_branch $CIRCLE_BRANCH)

I am sure that a change in workflow would require a decent discussion first so I am not opening a PR at this time and instead inviting discussion for conversation around what should be the right workflow.

ataylorme commented 5 years ago

Hi @ccjjmartin, Thanks for opening the issue and starting a discussion. We tried approaches similar to what you are doing, trimming the branch name into a multidev compatible name. The issue we found, however, is that there can be collisions. For example, fix/composer-item-1 and fix/composer-item-2 would both be trimmed to fix-compose.

I would suggest only using one approach to naming multidevs on Pantheon, ci- or pr- but not both in the same script.

The reference you have is to the 1.x tag of this Dockerfile. We have multiple versions, with the current version being 6.x, and have followed semantic versioning.

The current version, which Build Tools and our example repos use for new projects, sets things up so that only pull requests are built in order to avoid the race condition of an existing build on a ci- branch before a PR can be opened.

We did not change older versions, such as the referenced 1.x, when this was implemented in order to avoid breaking existing builds/projects.

If you have older projects and would like to discuss the differences between those and the current versions, as well as a migration path, I'm happy to schedule a call to do so.

ccjjmartin commented 5 years ago

Found it: https://github.com/pantheon-systems/docker-build-tools-ci/blob/6.x/scripts/set-environment#L46

Looks much better, the PR workflow makes sense. Last time I looked at this repo I think 4.x was the latest version but we were unable to upgrade because it dropped nodejs support and instead we had to use the nodejs branch / image. Was nodejs support added in 5.x or 6.x?

Have you considered having the master branch push up to test and a branch called develop push up to dev?

ccjjmartin commented 5 years ago

The README seems to indicate yes:

6.x: Use a CircleCI base image with Node JS

I will give it a shot and see how it goes.

ataylorme commented 5 years ago

Last time I looked at this repo I think 4.x was the latest version but we were unable to upgrade because it dropped nodejs support and instead we had to use the nodejs branch / image. Was nodejs support added in 5.x or 6.x?

The 6.x image is based on circleci/php:7.3-node-browsers so it has PHP, NodeJS and headless browsers from that plus Terminus, Build Tools , and the other items we add.

Also, you don't have to use the /build-tools-ci/scripts/set-environment script at all and are free to create your own. You will just need to ensure the proper environment variables, such as TERMINUS_ENV, get set.

ccjjmartin commented 5 years ago

Is there an example config.yml I can look at that uses the 6.x branch? I looked in the drops-8 repo but it looks like it is using 3.x. https://github.com/pantheon-systems/drops-8/blob/default/.circleci/config.yml#L3

ataylorme commented 5 years ago

pantheon-systems/example-drops-8-composer is the D8 example

ccjjmartin commented 5 years ago

@ataylorme Thanks for the help here. I ended up having to create a custom image based on this one. If you are interested in why:

1) This image was missing the bsdtar package and patternlab (an npm package wouldn't extract itself without it) 2) Node 10 wasn't working for me so I had to set node back to version 8. 3) The build tools plugin was giving me error messages, I think it is installed as part of the old config.yml process so I removed it here.

There are probably better ways to do what I did but here is the Dockerfile I created:

# Use an official Python runtime as a parent image
FROM quay.io/pantheon-public/build-tools-ci:6.x

# Switch to root user
USER root

RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.34.0/install.sh | bash

# Change to node 8
RUN echo "export NVM_DIR=\"$HOME/.nvm\"" >> .bashrc
RUN /bin/bash -c "source ~/.nvm/nvm.sh; nvm i lts/carbon; nvm alias default lts/carbon"
RUN rm /usr/local/bin/node; ln -s /root/.nvm/versions/node/v8.16.1/bin/node /usr/local/bin/node

RUN rm -Rf /usr/local/share/terminus-plugins/terminus-build-tools-plugin

# Install necessary packages for PHP extensions
RUN apt-get update && \
     apt-get install -y \
        bsdtar

Since I now have the upgraded version and I don't think it is creating the duplicate environments anymore, I am going to close this issue.