Better defaults for make setup with better testing of python. (#22804)

.github/workflows/ci.yml

@@ -88,7 +88,7 @@ jobs:
         shell: bash
         run: |
           docker compose version
-          npm exec jest -- ./tests/make --runInBand
+          make test_setup
 
   test_run_docker_action:
     runs-on: ubuntu-latest
@@ -158,6 +158,14 @@ jobs:
               exit 1
             fi
 
+      - name: Test setup
+        uses: ./.github/actions/run-docker
+        with:
+          digest: ${{ needs.build.outputs.digest }}
+          version: ${{ needs.build.outputs.version }}
+          run: |
+            pytest tests/make/
+
   docs_build:
     runs-on: ubuntu-latest
     needs: build

Makefile-os

@@ -7,7 +7,6 @@
 DOCKER_PROGRESS ?= auto
 DOCKER_METADATA_FILE ?= buildx-bake-metadata.json
 DOCKER_PUSH ?=
-export DEBUG ?= True
 export DOCKER_COMMIT ?=
 export DOCKER_BUILD ?=
 export DOCKER_VERSION ?=
@@ -68,6 +67,10 @@ help_submake:
 	@echo "\nAll other commands will be passed through to the docker 'web' container make:"
 	@make -f Makefile-docker help_submake
 
+.PHONY: test_setup
+test_setup:
+	npm exec jest -- ./tests/make --runInBand
+
 .PHONY: setup
 setup: ## create configuration files version.json and .env required to run this project
 	for path in $(CLEAN_PATHS); do rm -rf "$(PWD)/$$path" && echo "$$path removed"; done

scripts/setup.py

@@ -22,39 +22,44 @@ def get_env_file():
     return env
 
 
-env = get_env_file()
-
-
 def get_value(key, default_value):
     if key in os.environ:
         return os.environ[key]
 
-    if key in env:
-        return env[key]
+    from_file = get_env_file()
+
+    if key in from_file:
+        return from_file[key]
 
     return default_value
 
 
 def get_docker_tag():
-    image_name = 'mozilla/addons-server'
-    version = os.environ.get('DOCKER_VERSION')
-    digest = os.environ.get('DOCKER_DIGEST')
-
-    tag = f'{image_name}:local'
-
-    if digest:
-        tag = f'{image_name}@{digest}'
-    elif version:
-        tag = f'{image_name}:{version}'
-    else:
-        tag = get_value('DOCKER_TAG', tag)
-        # extract version or digest from existing tag
-        if '@' in tag:
-            digest = tag.split('@')[1]
-        elif ':' in tag:
-            version = tag.split(':')[1]
-
-    print('Docker tag: ', tag)
+    image = 'mozilla/addons-server'
+    version = 'local'
+
+    # First get the tag from the full tag variable
+    tag = get_value('DOCKER_TAG', f'{image}:{version}')
+    # extract version or digest from existing tag
+    if '@' in tag:
+        image, digest = tag.split('@')
+        version = None
+    elif ':' in tag:
+        image, version = tag.split(':')
+        digest = None
+
+    # DOCKER_DIGEST or DOCKER_VERSION can override the extracted version or digest
+    # Note: it will inherit the image from the provided DOCKER_TAG if also provided
+    if bool(os.environ.get('DOCKER_DIGEST', False)):
+        digest = os.environ['DOCKER_DIGEST']
+        tag = f'{image}@{digest}'
+        version = None
+    elif bool(os.environ.get('DOCKER_VERSION', False)):
+        version = os.environ['DOCKER_VERSION']
+        tag = f'{image}:{version}'
+        digest = None
+
+    print('tag: ', tag)
     print('version: ', version)
     print('digest: ', digest)
 
@@ -74,16 +79,48 @@ def get_docker_tag():
 # 3. the value defined in the environment variable
 # 4. the value defined in the make args.
 
-docker_tag, docker_version, docker_digest = get_docker_tag()
-
-docker_target = get_value('DOCKER_TARGET', 'development')
-compose_file = get_value('COMPOSE_FILE', ('docker-compose.yml'))
 
-set_env_file(
-    {
-        'COMPOSE_FILE': compose_file,
-        'DOCKER_TAG': docker_tag,
-        'DOCKER_TARGET': docker_target,
-        'HOST_UID': get_value('HOST_UID', os.getuid()),
-    }
-)
+def main():
+    docker_tag, docker_version, _ = get_docker_tag()
+
+    is_local = docker_version == 'local'
+
+    # The default target should be inferred from the version
+    # but can be freely overridden by the user.
+    # E.g running local image in production mode
+    docker_target = get_value(
+        'DOCKER_TARGET', ('development' if is_local else 'production')
+    )
+
+    is_production = docker_target == 'production'
+
+    # The default value for which compose files to use is based on the target
+    # but can be freely overridden by the user.
+    # E.g running a production image in development mode with source code changes
+    compose_file = get_value(
+        'COMPOSE_FILE',
+        (
+            'docker-compose.yml:docker-compose.ci.yml'
+            if is_production
+            else 'docker-compose.yml'
+        ),
+    )
+
+    # DEBUG is special, as we should allow the user to override it
+    # but we should not set a default to the previously set value but instead
+    # to the most sensible default.
+    debug = os.environ.get('DEBUG', str(False if is_production else True))
+
+    set_env_file(
+        {
+            'COMPOSE_FILE': compose_file,
+            'DOCKER_TAG': docker_tag,
+            'DOCKER_TARGET': docker_target,
+            'HOST_UID': get_value('HOST_UID', os.getuid()),
+            'DEBUG': debug,
+        }
+    )
+
+
+if __name__ == '__main__':
+    main()

tests/make/make.spec.js

@@ -8,24 +8,19 @@ const rootPath = path.join(__dirname, '..', '..');
 const envPath = path.join(rootPath, '.env');
 
 function runSetup(env) {
+  fs.writeFileSync(envPath, '');
   spawnSync('make', ['setup'], {
     env: { ...process.env, ...env },
     encoding: 'utf-8',
   });
-}
-
-function readEnvFile(name) {
-  return parse(fs.readFileSync(envPath, { encoding: 'utf-8' }))[name];
+  return parse(fs.readFileSync(envPath, { encoding: 'utf-8' }));
 }
 
 test('map docker compose config', () => {
-  const values = {
+  values = runSetup({
     DOCKER_VERSION: 'version',
     HOST_UID: 'uid',
-  };
-
-  fs.writeFileSync(envPath, '');
-  runSetup(values);
+  });
 
   const { stdout: rawConfig } = spawnSync(
     'docker',
@@ -36,19 +31,16 @@ test('map docker compose config', () => {
   const config = JSON.parse(rawConfig);
   const { web } = config.services;
 
-  expect(web.image).toStrictEqual(
-    `mozilla/addons-server:${values.DOCKER_VERSION}`,
-  );
+  expect(web.image).toStrictEqual(`mozilla/addons-server:version`);
   expect(web.platform).toStrictEqual('linux/amd64');
-  expect(web.environment.HOST_UID).toStrictEqual(values.HOST_UID);
+  expect(web.environment.HOST_UID).toStrictEqual('9500');
   expect(config.volumes.data_mysqld.name).toStrictEqual(
     'addons-server_data_mysqld',
   );
 });
 
 describe('docker-bake.hcl', () => {
   function getBakeConfig(env = {}) {
-    fs.writeFileSync(envPath, '');
     runSetup(env);
     const { stdout: output } = spawnSync(
       'make',
@@ -101,130 +93,3 @@ describe('docker-bake.hcl', () => {
     expect(output).toContain(`"target": "${target}"`);
   });
 });
-
-function standardPermutations(name, defaultValue) {
-  return [
-    {
-      name,
-      file: undefined,
-      env: undefined,
-      expected: defaultValue,
-    },
-    {
-      name,
-      file: 'file',
-      env: undefined,
-      expected: 'file',
-    },
-    {
-      name,
-      file: undefined,
-      env: 'env',
-      expected: 'env',
-    },
-    {
-      name,
-      file: 'file',
-      env: 'env',
-      expected: 'env',
-    },
-  ];
-}
-
-describe.each([
-  {
-    version: undefined,
-    digest: undefined,
-    tag: undefined,
-    expected: 'mozilla/addons-server:local',
-  },
-  {
-    version: 'version',
-    digest: undefined,
-    tag: undefined,
-    expected: 'mozilla/addons-server:version',
-  },
-  {
-    version: undefined,
-    digest: 'sha256:digest',
-    tag: undefined,
-    expected: 'mozilla/addons-server@sha256:digest',
-  },
-  {
-    version: 'version',
-    digest: 'sha256:digest',
-    tag: undefined,
-    expected: 'mozilla/addons-server@sha256:digest',
-  },
-  {
-    version: 'version',
-    digest: 'sha256:digest',
-    tag: 'previous',
-    expected: 'mozilla/addons-server@sha256:digest',
-  },
-  {
-    version: undefined,
-    digest: undefined,
-    tag: 'previous',
-    expected: 'previous',
-  },
-])('DOCKER_TAG', ({ version, digest, tag, expected }) => {
-  it(`version:${version}_digest:${digest}_tag:${tag}`, () => {
-    fs.writeFileSync(envPath, '');
-    runSetup({
-      DOCKER_VERSION: version,
-      DOCKER_DIGEST: digest,
-      DOCKER_TAG: tag,
-    });
-
-    const actual = readEnvFile('DOCKER_TAG');
-    expect(actual).toStrictEqual(expected);
-  });
-});
-
-const testCases = [
-  ...standardPermutations('DOCKER_TAG', 'mozilla/addons-server:local'),
-  ...standardPermutations('DOCKER_TARGET', 'development'),
-  ...standardPermutations('HOST_UID', process.getuid().toString()),
-  ...standardPermutations('COMPOSE_FILE', 'docker-compose.yml'),
-];
-
-describe.each(testCases)('.env file', ({ name, file, env, expected }) => {
-  it(`name:${name}_file:${file}_env:${env}`, () => {
-    fs.writeFileSync(envPath, file ? `${name}=${file}` : '');
-
-    runSetup({ [name]: env });
-
-    const actual = readEnvFile(name);
-    expect(actual).toStrictEqual(expected);
-  });
-});
-
-const testedKeys = new Set(testCases.map(({ name }) => name));
-
-// Keys testsed outside the scope of testCases
-const skippedKeys = ['DOCKER_COMMIT', 'DOCKER_VERSION', 'DOCKER_BUILD', 'PWD'];
-
-test('All dynamic properties in any docker compose file are referenced in the test', () => {
-  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];
-        if (!skippedKeys.includes(variable)) variableDefinitions.push(variable);
-      }
-    }
-  }
-
-  for (let variable of variableDefinitions) {
-    expect(testedKeys).toContain(variable);
-  }
-});