move build pipeline to circleci #4923

LeoMcA · 2021-10-14T15:17:01Z

Opened as a draft, because it's not backwards-compatible with Jenkins (as I'd hoped) because of the changes to Dockerfile and docker-compose.yml, so we can't merge until we're ready to remove Jenkins (and that requires figuring out the staticfiles -> s3 piece, or an alternative).

Comments welcome, a few steps in the Dockerfile might be a bit unusual or convoluted to allow more efficient caching - please point out if you're confused by anything so I know where to add comments.

.circleci/config.yml

akatsoulas

My first thought is to extract from this PR anything that can be merged into the existing implementation and affects Circle CI. After that just re-open this PR (minus the merges) in a new project in order to get some flexibility instead of pilling commits on top of a draft PR. Thoughts?

.circleci/config.yml

Dockerfile

LeoMcA · 2021-11-29T17:59:38Z

This should now be feature complete, after implementing s3 push and proper l10n management.

For the latter, I adjusted the approach we used in Jenkins which was:

pull mozilla-l10n/sumo-l10n repo
lint
if lint fails, abort
push to mozilla-it/sumo-l10n-prod repo
pull from mozilla-it/sumo-l10n-prod repo
compile

This approach lacked nuance, and had the effect of losing rather a lot of localisations if just one file failed linting.

The new approach in l10n-fetch-lint-compile.sh is something like:

pull mozilla-l10n/sumo-l10n repo
lint each file individually
if lint fails for a file, find what commit the file was last changed in, and checkout the file from that commit
lint file again, if it fails goto step 3
compile

This way if one locale has a failing file, the rest still keep their most up to date localisations.

Currently all our files pass linting (though msgfmt shows an error for some, which we should fix in the sumo-l10n repo, and then apply that same git backtracking logic to if msgfmt shows any errors as well), but you can test this locally by checking out a known bad commit in the sumo-l10n repo, then running the script:

cd locale
git checkout 3f359bd6868e3355f46fa81ea00f7edb9455e183
cd ../
./scripts/l10n-fetch-lint-compile.sh

You'll see a bunch of lint returned an error, trying 247e1c5812ef1a749130da96a3e41192912bff48 style messages, and running git status on the locale repo will show that the most recent "good" versions of the failing files have been checked out.

You can see a pretend main branch commit here: https://app.circleci.com/pipelines/github/mozilla/kitsune/1561/workflows/85bc7807-9bbd-4864-8f26-159d9e9e60bc/jobs/4283 (with a slightly adjusted config, to avoid uploading static files to the prod s3 bucket).

I think next steps are deploying a docker image from https://hub.docker.com/r/mozilla/kitsune/tags to stage to test the image is good.

akatsoulas

I still need to go through the locales but what's in this review won't be affected by that

akatsoulas · 2021-12-01T15:13:31Z

.circleci/config.yml

+ command: ./bin/dc.sh run test ./bin/run-mocha-tests.sh
+ - when:
+ condition:
+ equal: [ main, << pipeline.git.branch >> ]


Let's build the prod image on main branch and on any branch that starts with something like prod-

Building on this, how about splitting the jobs a bit more (lint, test-js, unittests, build). Then we can use the workflows to run them in parallel and have a conditional in the workflow to build only under circumstances (eg on main and prod-*)

how about splitting the jobs a bit more (lint, test-js, unittests, build).

This was my initial approach, however I couldn't immediately figure out how to share the docker context between each job, and so each had to build the image again completely from scratch - which took rather a long time. So I opted for the simpler (faster) approach first, to avoid this PR getting too unwieldily.

But my thought was, once merged, we can explore a bit more of the features Circle has to offer - like "proper" workflows, pretty test result output, and parallel tests.

.circleci/config.yml

akatsoulas · 2021-12-01T15:13:31Z

.circleci/config.yml

+ name: Build prod image
+ command: DOCKER_BUILDKIT=1 ./bin/dc.sh build --progress=plain prod
+ - run:
+ name: Push prod image


We could move Push prod image and Upload staticfiles outside of the job and treat them as reusable components

I'm not quite sure what you mean here

.circleci/config.yml

docker-compose.ci.yml

akatsoulas · 2021-12-01T15:13:31Z

docker-compose.ci.yml

+ - elasticsearch7
+ - redis
+
+ prod:


why is this needed?

To allow us to do docker-compose build prod and end up with an appropriately tagged image

akatsoulas · 2021-12-01T15:13:31Z

docker/bin/entrypoint.sh

@@ -1,5 +0,0 @@
-#!/bin/bash
-
-source /venv/bin/activate


isn't this needed any more? Is this replaced by something else?

I don't think we were/are using it anywhere!

This is used by mariadb in the docker-file

I'm pretty sure that's coming from inside the mariadb image (especially since it's called docker-entrypoint.sh not just entrypoint.sh)

docker/bin/upload-staticfiles.sh

kitsune/sumo/jinja2/base.html

LeoMcA · 2021-12-02T14:30:02Z

The prod- conditional is working nicely: https://app.circleci.com/pipelines/github/mozilla/kitsune/1565/workflows/1899ab5e-cd4f-492c-ba53-6d944c90b9ed/jobs/4290/steps

akatsoulas · 2021-12-02T17:10:29Z

@smarnach could you have a look at this PR? It's mostly done but it would be nice if we could get a second pair of eyes before merging.

smarnach

The changes here look good from an ops perspective.

It's not quite clear to me how the condition for pushing to Docker Hub works. If I look at the build for the last commit in this PR, it looks like it pushed a new tag to Docker Hub. If I look at the condition, I think it shouldn't have. What's the idea here?

smarnach · 2021-12-06T13:40:08Z

.circleci/config.yml

@@ -10,22 +12,59 @@ jobs:
 command: |
 sudo pip install --upgrade pre-commit==2.15.0
 pre-commit run --all-files
+ test-and-build:
+ docker:
+ - image: circleci/python:3.9-buster


Maybe use the 3.9-bullseye tag, which seems closer to what's used inside the Docker image? Probably doesn't make much of a difference.

akatsoulas · 2021-12-06T14:43:32Z

The changes here look good from an ops perspective.

It's not quite clear to me how the condition for pushing to Docker Hub works. If I look at the build for the last commit in this PR, it looks like it pushed a new tag to Docker Hub. If I look at the condition, I think it shouldn't have. What's the idea here?

The idea here is to push to docker-hub always from the main branch but also have the ability to push an image in different branches. In Jenkins there is an image pushed to every branch. In circle-ci we agreed that everything that starts with prod- should trigger an image push.

smarnach · 2021-12-06T14:53:57Z

The idea here is to push to docker-hub always from the main branch but also have the ability to push an image in different branches. In Jenkins there is an image pushed to every branch. In circle-ci we agreed that everything that starts with prod- should trigger an image push.

That's how I read the condition as well. I still wonder why the last build for this PR pushed an image, since this branch is called new-build-pipeline. Shouldn't this only push an image once merged?

akatsoulas · 2021-12-06T15:10:42Z

I believe that condition was added as part of the review process which also was the latest commit that was built from both the new-build-pipeline and the prod-new-build-pipeline branches. The image hash though in the docker repo seems to be coming from the prod-new-build-pipeline branch. That said, I believe it's worth double checking this by triggering one more build in the current branch and make sure that it won't push to docker hub.

akatsoulas

There are a few comments that require changes but overall this is looking good. Nice work!
I am more than happy to have another pass if you would like for the new changes. r+wc

akatsoulas · 2021-12-06T15:39:00Z

docker/bin/entrypoint.sh

@@ -1,5 +0,0 @@
-#!/bin/bash
-
-source /venv/bin/activate


This is used by mariadb in the docker-file

docker/bin/upload-staticfiles.sh

.circleci/config.yml

Dockerfile

.circleci/config.yml

akatsoulas · 2021-12-06T15:39:00Z

scripts/l10n-fetch-lint-compile.sh

+echo "po status for commit ${head_hash}:" > $postatus_file
+echo -e "(if nothing appears below, no errors were detected)\\n" >> $postatus_file
+
+function lintpo() {


What happens if subsequent commits have errors?

The script will keep backtracking until it finds a file which passes linting - the beauty of doing this on a git repo is that since all the files right now pass linting, we know that there'll always be a file we can (eventually) get back to which passes. That may take rather a while in the worst case, but if we notice there's a locale which is failing for a few builds in a row we can always submit a PR to fix the error.

LeoMcA · 2021-12-07T11:31:06Z

The image hash though in the docker repo seems to be coming from the prod-new-build-pipeline branch. That said, I believe it's worth double checking this by triggering one more build in the current branch and make sure that it won't push to docker hub.

Yes I think this is a quirk of the circleci/github integration - since I've pushed the same commits to both the new-build-pipeline and prod-new-build-pipeline branches, it's linking to the latter from this commit (I guess because I pushed there first?)

The latest commit is only on this branch, so we should see this stopping after tests: https://circleci.com/gh/mozilla/kitsune/4296

I've deployed https://hub.docker.com/layers/mozilla/kitsune/prod-8b6e26/images/sha256-eec231404190c0018f85e3e85739c2ed76889c52489d4d00ba388c5a01346985?context=explore to stage and everything seems to be working fine!

smarnach · 2021-12-07T13:39:45Z

Yes I think this is a quirk of the circleci/github integration - since I've pushed the same commits to both the new-build-pipeline and prod-new-build-pipeline branches, it's linking to the latter from this commit (I guess because I pushed there first?)

Thanks for the explanation. I think Circle CI only runs once for each commit, and the builds are linked by commit hash, so if you pushed on the prod-… branch first that would explain the behaviour.

LeoMcA requested review from smarnach and akatsoulas October 14, 2021 15:17

LeoMcA commented Oct 14, 2021

View changes

.circleci/config.yml Outdated Show resolved Hide resolved

akatsoulas reviewed Oct 15, 2021

View changes

.circleci/config.yml Show resolved Hide resolved

mozilla deleted a comment from Am0504050720 Oct 18, 2021

akatsoulas reviewed Nov 4, 2021

View changes

Dockerfile Show resolved Hide resolved

akatsoulas reviewed Nov 4, 2021

View changes

Dockerfile Show resolved Hide resolved

akatsoulas reviewed Nov 4, 2021

View changes

Dockerfile Outdated Show resolved Hide resolved

LeoMcA force-pushed the new-build-pipeline branch 3 times, most recently from 79a673f to f8dafe9 Compare November 15, 2021 17:35

LeoMcA force-pushed the new-build-pipeline branch 3 times, most recently from 4336f6d to 22fb209 Compare November 24, 2021 18:08

move build pipeline to circleci

24694be

LeoMcA force-pushed the new-build-pipeline branch from 22fb209 to 24694be Compare November 29, 2021 17:36

LeoMcA requested a review from akatsoulas November 29, 2021 17:59

akatsoulas marked this pull request as ready for review November 30, 2021 10:40

akatsoulas requested changes Dec 1, 2021

View changes

review updates

7acc1c9

update docker hub references to new repo/image name

2f692d7

smarnach approved these changes Dec 6, 2021

View changes

akatsoulas approved these changes Dec 6, 2021

View changes

further review updates

8b6e267

upload prod staticfiles as well

e62eb72

LeoMcA merged commit 2871ee4 into main Dec 7, 2021
2 checks passed

LeoMcA deleted the new-build-pipeline branch December 7, 2021 11:57

move build pipeline to circleci #4923

move build pipeline to circleci #4923

LeoMcA commented Oct 14, 2021 •

edited

akatsoulas left a comment

LeoMcA commented Nov 29, 2021

akatsoulas left a comment

akatsoulas Dec 1, 2021

akatsoulas Dec 1, 2021

LeoMcA Dec 1, 2021

akatsoulas Dec 1, 2021

LeoMcA Dec 2, 2021

akatsoulas Dec 1, 2021

LeoMcA Dec 1, 2021

akatsoulas Dec 1, 2021

LeoMcA Dec 1, 2021 •

edited

akatsoulas Dec 6, 2021

LeoMcA Dec 7, 2021

LeoMcA commented Dec 2, 2021

akatsoulas commented Dec 2, 2021

smarnach left a comment

smarnach Dec 6, 2021

akatsoulas commented Dec 6, 2021

smarnach commented Dec 6, 2021

akatsoulas commented Dec 6, 2021

akatsoulas left a comment

akatsoulas Dec 6, 2021

akatsoulas Dec 6, 2021

LeoMcA Dec 7, 2021

LeoMcA commented Dec 7, 2021

smarnach commented Dec 7, 2021

move build pipeline to circleci #4923

move build pipeline to circleci #4923

Conversation

LeoMcA commented Oct 14, 2021 • edited

akatsoulas left a comment

Choose a reason for hiding this comment

LeoMcA commented Nov 29, 2021

akatsoulas 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

LeoMcA Dec 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeoMcA commented Dec 2, 2021

akatsoulas commented Dec 2, 2021

smarnach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akatsoulas commented Dec 6, 2021

smarnach commented Dec 6, 2021

akatsoulas commented Dec 6, 2021

akatsoulas 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

LeoMcA commented Dec 7, 2021

smarnach commented Dec 7, 2021

LeoMcA commented Oct 14, 2021 •

edited

LeoMcA Dec 1, 2021 •

edited