"Fix makefile env variables definition" (#22246)

* TMP: refactor buildx bake definition and test * TMP: better initialization and organization of make file env vars (docker relevant +) * TMP: cleaner code. * TMP: remove build
2024-05-16 14:49:37 +02:00 · 2024-05-16 14:49:37 +02:00 · c4f4ba6487
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@ -368,6 +368,12 @@ commands:
            echo 'export VERSION_BUILD_URL=$CIRCLE_BUILD_URL' >> $BASH_ENV
            echo 'export DOCKER_PUSH=<< parameters.push >>' >> $BASH_ENV
            echo 'export DOCKER_OUTPUT=<< parameters.output_file >>' >> $BASH_ENV
+      - run:
+          name: Create .env file
+          command: |
+            # We must defined a .env file for the docker-compose config
+            # The values will be inferred by what is set in the bash env above.
+            make create_env_file
      - run:
          name: Docker build config
          command: |
--- a/.github/actions/build-docker/action.yml
+++ b/.github/actions/build-docker/action.yml
@ -73,13 +73,21 @@ runs:
          # The production build
          # type=raw,value=latest,enable={{is_default_branch}}

+    - name: Define environment variables
+      shell: bash
+      run: |
+        echo "DOCKER_VERSION=${{ steps.meta.outputs.version }}" >> $GITHUB_ENV
+        echo "DOCKER_COMMIT=${{ github.sha }}" >> $GITHUB_ENV
+        echo "VERSION_BUILD_URL=${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" >> $GITHUB_ENV
+
+    - name: Create .env file
+      shell: bash
+      run: make create_env_file
+
    - name: Create version
      shell: bash
      run: |
-        make version \
-          DOCKER_VERSION="${{ steps.meta.outputs.version }}" \
-          DOCKER_COMMIT="${{ github.sha }}" \
-          VERSION_BUILD_URL="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}"
+        make version

    - name: Build Image
      uses: docker/bake-action@v4
@ -87,5 +95,3 @@ runs:
        targets: web
        push: ${{ inputs.push }}
        load: ${{ inputs.push == 'false' }}
-      env:
-        DOCKER_VERSION: ${{ steps.meta.outputs.version }}
--- a/.github/workflows/verify-docker-image.yml
+++ b/.github/workflows/verify-docker-image.yml
@ -8,45 +8,18 @@ on:
 jobs:
  docker_config_check:
    runs-on: ubuntu-latest
-    strategy:
-      matrix:
-        include:
-          - expected: { version: local, push: false }
-            input: { version: '', push: '' }
-          - expected: { version: version, push: true }
-            input: { version: version, push: true }

    steps:
      - uses: actions/checkout@v4
-
-      - name: Check Docker Compose (default)
-        id: default
-        env:
-          DOCKER_VERSION: ${{ matrix.input.version }}
-          DOCKER_PUSH: ${{ matrix.input.push }}
+      - uses: actions/setup-node@v2
+      - name: Install dependencies
+        shell: bash
+        run: npm ci
+      - name: Check make/docker configuration
        shell: bash
        run: |
-          set -xue
-          config=$(make docker_compose_config)
-
-          # Expect image tag is correct
-          echo $config | grep -q "image: mozilla/addons-server:${{ matrix.expected.version }}"
-
-          # Expect docker push args are correct
-          if [[ ${{ matrix.expected.push }} == "true" ]]; then
-            echo $config | grep -q -- "--push"
-            echo $config | grep -v -q -- "--load"
-          else
-            echo $config | grep -v -q -- "--push"
-            echo $config | grep -q -- "--load"
-          fi
-
-          # Expect docker platform is correct
-          echo $config | grep -q -- "platform: linux/amd64"
-          echo $config | grep -q -- "\"platforms\": \[
-            \"linux/amd64\"
-          \]"
-
+          docker compose version
+          npm exec jest -- ./tests/make --runInBand

  verify_docker_image:
    runs-on: ubuntu-latest
--- a/2
+++ b/2
@ -263,7 +263,7 @@ test_failed: ## rerun the failed tests from the previous run

 .PHONY: run_js_tests
 run_js_tests: ## Run the JavaScript test suite (requires compiled/compressed assets).
-	NODE_PATH=$(NODE_MODULES) npm exec $(NPM_ARGS) -- jest
+	NODE_PATH=$(NODE_MODULES) npm exec $(NPM_ARGS) -- jest tests/js

 .PHONY: watch_js_tests
 watch_js_tests: ## Run+watch the JavaScript test suite (requires compiled/compressed assets).
--- a/71
+++ b/71
@ -1,24 +1,33 @@
-export HOST_UID := $(shell id -u)
+####################################################################################################
+# Our makefile makes use of docker compose commands. Our config files rely on environment variables
+# both for passing configuration to the containers as well as configuring the compose file itself.
+# Variables referenced in docker-compose*.yml should be read from .env, exported and saved in .env

-export DOCKER_BUILDER=container
+define ENV_VAR
+export $(1) ?= $(or $(shell test -f .env && grep -w $(1) .env | cut -d '=' -f2), $(2))
+endef

+# This value can be hardcoded, but should still be synced to the .env file
+override DOCKER_MYSQLD_VOLUME = addons-server_data_mysqld
+$(eval $(call ENV_VAR,DOCKER_VERSION,$(shell echo local)))
+$(eval $(call ENV_VAR,HOST_UID,$(shell id -u)))
+$(eval $(call ENV_VAR,SUPERUSER_EMAIL,$(shell git config user.email || echo admin@mozilla.com)))
+$(eval $(call ENV_VAR,SUPERUSER_USERNAME,$(shell git config user.name || echo admin)))
+
+####################################################################################################
+
+DOCKER_BUILDER ?= container
 DOCKER_PROGRESS ?= auto
 DOCKER_PUSH ?= false
 DOCKER_OUTPUT ?=
 DOCKER_COMMIT ?= $(shell git rev-parse HEAD || echo "commit")
 VERSION_BUILD_URL ?= build
-export DOCKER_MYSQLD_VOLUME = addons-server_data_mysqld
+BUILDX_BAKE_COMMAND := docker buildx bake web

-BACKUPS_DIR = $(shell pwd)/backups
-EXPORT_DIR = $(BACKUPS_DIR)/$(shell date +%Y%m%d%H%M%S)
+override BACKUPS_DIR = $(shell pwd)/backups
+override EXPORT_DIR = $(BACKUPS_DIR)/$(shell date +%Y%m%d%H%M%S)
 RESTORE_DIR ?= $(BACKUPS_DIR)/$(shell ls -1 backups | sort -r | head -n 1)

-# Exporting these variables make them default values for docker-compose*.yml files
-export DOCKER_VERSION ?= local
-
-# define the username and email for the superuser in the container
-SUPERUSER_EMAIL=$(shell git config user.email)
-SUPERUSER_USERNAME=$(shell git config user.name)

 .PHONY: help_redirect
 help_redirect:
@ -31,6 +40,15 @@ help_submake:
 	@echo "\nAll other commands will be passed through to the docker 'web' container make:"
 	@make -f Makefile-docker help_submake

+.PHONY: create_env_file
+create_env_file:
+	@ rm -f .env
+	echo "DOCKER_VERSION=${DOCKER_VERSION}" >> .env
+	echo "HOST_UID=${HOST_UID}" >> .env
+	echo "SUPERUSER_EMAIL=${SUPERUSER_EMAIL}" >> .env
+	echo "SUPERUSER_USERNAME=${SUPERUSER_USERNAME}" >> .env
+	echo "DOCKER_MYSQLD_VOLUME=${DOCKER_MYSQLD_VOLUME}" >> .env
+
 .PHONY: push_locales
 push_locales: ## extracts and merges translation strings
 	bash ./scripts/push_l10n_extraction.sh $(ARGS)
@ -46,13 +64,6 @@ shell: ## connect to a running addons-server docker shell
 rootshell: ## connect to a running addons-server docker shell with root user
 	docker compose exec --user root web bash

-.PHONY: create_env_file
-create_env_file:
-	@ rm -f .env
-	echo "HOST_UID=${HOST_UID}" >> .env
-	echo "SUPERUSER_EMAIL=${SUPERUSER_EMAIL}" >> .env
-	echo "SUPERUSER_USERNAME=${SUPERUSER_USERNAME}" >> .env
-
 .PHONY: data_export
 data_export:
 	@ mkdir -p $(EXPORT_DIR)
@ -79,18 +90,18 @@ create_docker_builder: ## Create a custom builder for buildkit to efficiently bu
 		--name $(DOCKER_BUILDER) \
 		--driver=docker-container

-DOCKER_BUILD_ARGS := \
+BUILDX_BAKE_COMMAND += \
 --progress=$(DOCKER_PROGRESS) \
 --builder=$(DOCKER_BUILDER) \

 ifeq ($(DOCKER_PUSH), true)
-	DOCKER_BUILD_ARGS += --push
+	BUILDX_BAKE_COMMAND += --push
 else
-	DOCKER_BUILD_ARGS += --load
+	BUILDX_BAKE_COMMAND += --load
 endif

 ifneq ($(DOCKER_OUTPUT),)
-	DOCKER_BUILD_ARGS += --metadata-file=$(DOCKER_OUTPUT)
+	BUILDX_BAKE_COMMAND += --metadata-file=$(DOCKER_OUTPUT)
 endif

 .PHONY: version
@ -99,15 +110,19 @@ version: ## create version.json file

 .PHONY: docker_compose_config
 docker_compose_config: ## Show the docker compose configuration
-	@echo "version: $(DOCKER_VERSION)"
-	@echo "push: $(DOCKER_PUSH)"
-	docker compose config web
-	docker buildx bake web --print
-	echo $(DOCKER_BUILD_ARGS)
+	@docker compose config web --format json
+
+.PHONY: docker_build_args
+docker_build_args: ## Show the docker build configuration
+	@echo $(BUILDX_BAKE_COMMAND)
+
+.PHONY: docker_build_config
+docker_build_config:
+	@$(BUILDX_BAKE_COMMAND) --print

 .PHONY: build_docker_image
 build_docker_image: create_docker_builder version ## Build the docker image
-	docker buildx bake web $(DOCKER_BUILD_ARGS)
+	$(BUILDX_BAKE_COMMAND)

 .PHONY: docker_mysqld_volume_create
 docker_mysqld_volume_create: ## Create the mysqld volume
--- a/docker-compose.yml
+++ b/docker-compose.yml
@ -1,4 +1,7 @@
 x-env-mapping: &env
+  # https://docs.docker.com/compose/environment-variables/envvars-precedence/
+  env_file:
+    - .env
  environment:
    - CELERY_BROKER_URL=amqp://olympia:olympia@rabbitmq/olympia
    - CELERY_RESULT_BACKEND=redis://redis:6379/1
@ -12,23 +15,19 @@ x-env-mapping: &env
    - PYTHONUNBUFFERED=1
    - PYTHONBREAKPOINT=ipdb.set_trace
    - TERM=xterm-256color
-    - CIRCLECI=${CIRCLECI}
    - HISTFILE=/data/olympia/docker/artifacts/bash_history
    - HISTSIZE=50000
    - HISTIGNORE=ls:exit:"cd .."
    - HISTCONTROL=erasedups
-    # Note: docker compose uses the values exported from .env for HOST_UID if
-    # it exists. ./docker/entrypoint.sh uses this variable to fix
-    # the uid of the olympia user to match the host user id if necessary.
-    - HOST_UID=${HOST_UID:-}
-    # This is the email address of the superuser created when initializing the db
-    - SUPERUSER_EMAIL=${SUPERUSER_EMAIL:-admin@mozilla.com}
-    - SUPERUSER_USERNAME=${SUPERUSER_USERNAME:-admin}
+    - CIRCLECI
+    - HOST_UID
+    - SUPERUSER_EMAIL
+    - SUPERUSER_USERNAME

 services:
  worker: &worker
    <<: *env
-    image: mozilla/addons-server:${DOCKER_VERSION:-local}
+    image: mozilla/addons-server:${DOCKER_VERSION:-}
    build:
      context: .
      dockerfile: Dockerfile
@ -169,4 +168,4 @@ volumes:
    driver_opts:
      type: none
      o: bind
-      device: ${PWD}/storage
+      device: ./storage
--- a/jest.config.js
+++ b/jest.config.js
@ -2,6 +2,6 @@
 // https://jestjs.io/docs/en/configuration.html
 module.exports = {
  setupFiles: ['<rootDir>/tests/js/setup.js'],
-  testMatch: ['<rootDir>/tests/js/**/*.spec.js'],
+  testMatch: ['<rootDir>/tests/**/*.spec.js'],
  testEnvironment: 'jsdom',
 };
--- a/package-lock.json
+++ b/package-lock.json
@ -26,6 +26,7 @@
        "underscore": "1.13.6"
      },
      "devDependencies": {
+        "dotenv": "^16.4.5",
        "jest": "^29.7.0",
        "jest-date-mock": "^1.0.10",
        "jest-environment-jsdom": "^29.7.0",
@ -2702,6 +2703,18 @@
        "url": "https://github.com/fb55/domutils?sponsor=1"
      }
    },
+    "node_modules/dotenv": {
+      "version": "16.4.5",
+      "resolved": "https://registry.npmjs.org/dotenv/-/dotenv-16.4.5.tgz",
+      "integrity": "sha512-ZmdL2rui+eB2YwhsWzjInR8LldtZHGDoQ1ugH85ppHKwpUHL7j7rN0Ti9NCnGiQbhaZ11FpR+7ao1dNsmduNUg==",
+      "dev": true,
+      "engines": {
+        "node": ">=12"
+      },
+      "funding": {
+        "url": "https://dotenvx.com"
+      }
+    },
    "node_modules/eastasianwidth": {
      "version": "0.2.0",
      "resolved": "https://registry.npmjs.org/eastasianwidth/-/eastasianwidth-0.2.0.tgz",
@ -8735,6 +8748,12 @@
        "domhandler": "^5.0.1"
      }
    },
+    "dotenv": {
+      "version": "16.4.5",
+      "resolved": "https://registry.npmjs.org/dotenv/-/dotenv-16.4.5.tgz",
+      "integrity": "sha512-ZmdL2rui+eB2YwhsWzjInR8LldtZHGDoQ1ugH85ppHKwpUHL7j7rN0Ti9NCnGiQbhaZ11FpR+7ao1dNsmduNUg==",
+      "dev": true
+    },
    "eastasianwidth": {
      "version": "0.2.0",
      "resolved": "https://registry.npmjs.org/eastasianwidth/-/eastasianwidth-0.2.0.tgz",
--- a/package.json
+++ b/package.json
@ -24,6 +24,7 @@
    "underscore": "1.13.6"
  },
  "devDependencies": {
+    "dotenv": "^16.4.5",
    "jest": "^29.7.0",
    "jest-date-mock": "^1.0.10",
    "jest-environment-jsdom": "^29.7.0",
--- a/tests/make/make.spec.js
+++ b/tests/make/make.spec.js
@ -0,0 +1,208 @@
+const { spawnSync } = require('child_process');
+const path = require('path');
+const fs = require('fs');
+const { globSync } = require('glob');
+
+const rootPath = path.join(__dirname, '..', '..');
+const envPath = path.join(rootPath, '.env');
+
+const options = [false, true];
+const frozenKeys = ['DOCKER_MYSQLD_VOLUME'];
+const configurableKeys = [
+  'DOCKER_VERSION',
+  'HOST_UID',
+  'SUPERUSER_EMAIL',
+  'SUPERUSER_USERNAME',
+];
+const keys = [...frozenKeys, ...configurableKeys];
+
+const product = (...arrays) =>
+  arrays.reduce((a, b) => a.flatMap((d) => b.map((e) => [d, e].flat())));
+
+const runCommand = (args, env = {}) => {
+  const result = spawnSync('make', ['-f', 'Makefile-os', ...args], {
+    env: {
+      ...process.env,
+      ...env,
+    },
+    encoding: 'utf-8',
+  });
+  if (result.status !== 0) throw new Error(result.stderr);
+  return result;
+};
+
+const readEnv = () =>
+  fs.existsSync(envPath)
+    ? require('dotenv').parse(fs.readFileSync(envPath, { encoding: 'utf-8' }))
+    : require('dotenv').parse('');
+
+const cleanEnv = () => {
+  if (fs.existsSync(envPath)) fs.rmSync(envPath);
+  const env = readEnv();
+  for (const key of keys) delete env[key];
+
+  return env;
+};
+
+const runMakeCreateEnvFile = (
+  name,
+  useFile = false,
+  useEnv = false,
+  useArgs = false,
+) => {
+  const env = cleanEnv();
+  const args = ['create_env_file'];
+
+  if (useFile) fs.writeFileSync(envPath, `${name}=file`);
+  if (useEnv) env[name] = 'env';
+  if (useArgs) args.push(`${name}=args`);
+
+  runCommand(args, env);
+  const result = readEnv()[name];
+
+  console.debug(`
+    name: ${name}
+    args: ${JSON.stringify({ useFile, useEnv, useArgs }, null, 2)}
+    command: make ${args.join(' ')}
+    env: ${env[name] || 'undefined'}
+    envFile: ${readEnv()[name] || 'undefined'}
+    result: ${result}
+  `);
+
+  cleanEnv();
+  return result;
+};
+
+const defaultValues = keys.reduce((acc, key) => {
+  acc[key] = runMakeCreateEnvFile(key);
+  return acc;
+}, {});
+
+describe('environment based configurations', () => {
+  beforeEach(cleanEnv);
+
+  describe.each(product(options, options, options, options, options))(
+    'test_docker_compose_config',
+    (useVersion, usePush, useUid, useEmail, useName) => {
+      it(`
+      test_version:${useVersion}_push:${usePush}_uid:${useUid}_email:${useEmail}_name:${useName}
+    `, () => {
+        const version = useVersion ? 'version' : defaultValues.DOCKER_VERSION;
+        const uid = useUid ? '1000' : defaultValues.HOST_UID;
+        const email = useEmail ? 'email' : defaultValues.SUPERUSER_EMAIL;
+        const userName = useName ? 'name' : defaultValues.SUPERUSER_USERNAME;
+
+        const args = ['docker_compose_config'];
+
+        if (useVersion) args.push(`DOCKER_VERSION=${version}`);
+        if (useUid) args.push(`HOST_UID=${uid}`);
+        if (useEmail) args.push(`SUPERUSER_EMAIL=${email}`);
+        if (useName) args.push(`SUPERUSER_USERNAME=${userName}`);
+
+        runCommand(['create_env_file']);
+        const result = runCommand(args);
+        const {
+          services: { web },
+        } = JSON.parse(result.stdout);
+
+        expect(web.image).toStrictEqual(`mozilla/addons-server:${version}`);
+        expect(web.platform).toStrictEqual('linux/amd64');
+        expect(web.environment.HOST_UID).toStrictEqual(uid);
+        expect(web.environment.SUPERUSER_EMAIL).toStrictEqual(email);
+        expect(web.environment.SUPERUSER_USERNAME).toStrictEqual(userName);
+
+        const builder = 'test_builder';
+        const progress = 'test_progress';
+
+        let expectedBuildargs = `docker buildx bake web --progress=${progress} --builder=${builder}`;
+        const dockerBuildArgs = [
+          'docker_build_args',
+          `DOCKER_BUILDER=${builder}`,
+          `DOCKER_PROGRESS=${progress}`,
+        ];
+
+        if (usePush) {
+          expectedBuildargs += ' --push';
+          dockerBuildArgs.push('DOCKER_PUSH=true');
+        } else {
+          expectedBuildargs += ' --load';
+        }
+
+        const { stdout: actualBuildArgs } = runCommand(dockerBuildArgs);
+        expect(actualBuildArgs.trim()).toContain(expectedBuildargs.trim());
+
+        const { stdout: bakeConfigOutput } = runCommand([
+          'docker_build_config',
+        ]);
+        const bakeConfig = JSON.parse(bakeConfigOutput);
+        expect(bakeConfig.target.web.platforms).toStrictEqual(['linux/amd64']);
+      });
+    },
+  );
+
+  test('docker compose substitution', () => {
+    expect(new Set(Object.keys(defaultValues))).toStrictEqual(new Set(keys));
+
+    const composeFiles = globSync('docker-compose*.yml', { cwd: rootPath });
+    const variableDefinitions = [];
+
+    for (let file of composeFiles) {
+      const fileContent = fs.readFileSync(path.join(rootPath, file), {
+        encoding: 'utf-8',
+      });
+
+      for (let line of fileContent.split('\n')) {
+        const regex = /\${(.*?)(?::-.*)?}/g;
+        let match;
+        while ((match = regex.exec(line)) !== null) {
+          const variable = match[1];
+          variableDefinitions.push(variable);
+        }
+      }
+    }
+
+    for (let variable of variableDefinitions) {
+      expect(keys).toContain(variable);
+    }
+  });
+
+  describe.each(product(configurableKeys, options, options, options))(
+    'test_configurable_keys',
+    (name, useFile, useEnv, useArgs) => {
+      it(`test_${name}_file:${useFile}_env:${useEnv}_args:${useArgs}`, () => {
+        let expectedValue = defaultValues[name];
+
+        if (!expectedValue) throw new Error(`expected value for ${name}.`);
+
+        if (useFile) expectedValue = 'file';
+        if (useEnv) expectedValue = 'env';
+        if (useArgs) expectedValue = 'args';
+
+        const actualValue = runMakeCreateEnvFile(
+          name,
+          useFile,
+          useEnv,
+          useArgs,
+        );
+
+        expect(actualValue).toStrictEqual(expectedValue);
+      });
+    },
+  );
+
+  describe.each(product(frozenKeys, options, options, options))(
+    'test_frozen_keys',
+    (name, useFile, useEnv, useArgs) => {
+      it(`test_${name}_file:${useFile}_env:${useEnv}_args:${useArgs}`, () => {
+        const expectedValue = defaultValues[name];
+        const actualValue = runMakeCreateEnvFile(
+          name,
+          useFile,
+          useEnv,
+          useArgs,
+        );
+        expect(actualValue).toStrictEqual(expectedValue);
+      });
+    },
+  );
+});