From 03e8eab7c8a20d56568819fd4ae9e3c7e10d8ff1 Mon Sep 17 00:00:00 2001 From: Kevin Meinhardt Date: Thu, 12 Dec 2024 18:10:24 +0100 Subject: [PATCH] Add the CI tests and watch it burn --- .github/actions/run-docker/action.yml | 14 ++- Makefile-docker | 2 +- Makefile-os | 34 ++++--- docker-compose.yml | 4 + docs/topics/development/data_management.md | 6 +- scripts/setup.py | 33 ++++--- src/olympia/core/apps.py | 14 ++- tests/make/test_setup.py | 101 ++++++++++++++++++++- 8 files changed, 170 insertions(+), 38 deletions(-) diff --git a/.github/actions/run-docker/action.yml b/.github/actions/run-docker/action.yml index 1619f498eb95..b969f518d529 100644 --- a/.github/actions/run-docker/action.yml +++ b/.github/actions/run-docker/action.yml @@ -15,8 +15,8 @@ inputs: logs: description: 'Show logs' required: false - data_backup_skip: - description: 'Skip data backup' + initialize: + description: 'Run the initialize script' required: false default: 'true' target: @@ -36,6 +36,8 @@ runs: using: 'composite' steps: - name: Run Docker Container + id: run + continue-on-error: true shell: bash run: | # Start the specified services @@ -52,11 +54,13 @@ runs: # Exec the run command in the container # quoted 'EOF' to prevent variable expansion - cat <<'EOF' | docker compose exec --user olympia web sh + cat <<'EOF' | docker compose exec --user olympia web bash ${{ inputs.run }} EOF - name: Logs - if: ${{ inputs.logs }} shell: bash - run: docker compose logs + if: ${{ steps.run.outcome == 'failure' }} + run: | + docker compose logs + exit 1 diff --git a/Makefile-docker b/Makefile-docker index 87c2ef8c6bc2..708e5fac1b66 100644 --- a/Makefile-docker +++ b/Makefile-docker @@ -69,7 +69,7 @@ data_dump: .PHONY: data_load data_load: - ./manage.py data_load $(ARGS) + ./manage.py data_load --name $(LOAD) .PHONY: update_assets update_assets: diff --git a/Makefile-os b/Makefile-os index e3079503fcd3..2c08edd6140c 100644 --- a/Makefile-os +++ b/Makefile-os @@ -18,16 +18,8 @@ export DATA_BACKUP_SKIP ?= override DOCKER_MYSQLD_VOLUME = addons-server_data_mysqld INITIALIZE_ARGS ?= -INIT_CLEAN ?= -INIT_LOAD ?= - -ifneq ($(INIT_CLEAN),) - INITIALIZE_ARGS += --clean -endif - -ifneq ($(INIT_LOAD),) - INITIALIZE_ARGS += --load $(INIT_LOAD) -endif +export CLEAN ?= +export LOAD ?= DOCKER_BAKE_ARGS := \ --file docker-bake.hcl \ @@ -35,15 +27,29 @@ DOCKER_BAKE_ARGS := \ --progress $(DOCKER_PROGRESS) \ --metadata-file $(DOCKER_METADATA_FILE) \ -ifeq ($(DOCKER_PUSH), true) - DOCKER_BAKE_ARGS += --push -endif - DOCKER_COMPOSE_ARGS := \ -d \ --remove-orphans \ --no-build \ --quiet-pull \ + --timestamps + +ifneq ($(CLEAN),) + # Build without cache + DOCKER_COMPOSE_ARGS += --no-cache + # Force recreate all containers + DOCKER_COMPOSE_ARGS += --force-recreate + # Clean the database and all other data when Initializing + INITIALIZE_ARGS += --clean +endif + +ifneq ($(LOAD),) + INITIALIZE_ARGS += --load $(LOAD) +endif + +ifeq ($(DOCKER_PUSH), true) + DOCKER_BAKE_ARGS += --push +endif ifneq ($(DOCKER_WAIT),) DOCKER_COMPOSE_ARGS += --wait diff --git a/docker-compose.yml b/docker-compose.yml index 4e847cacc989..89a30a48a309 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -163,10 +163,14 @@ services: redis: image: redis:6.2 + volumes: + - data_redis:/data rabbitmq: image: rabbitmq:3.12 hostname: olympia + volumes: + - data_rabbitmq:/var/lib/rabbitmq expose: - "5672" environment: diff --git a/docs/topics/development/data_management.md b/docs/topics/development/data_management.md index 8d46d00c87d5..4bd9cc845dfa 100644 --- a/docs/topics/development/data_management.md +++ b/docs/topics/development/data_management.md @@ -27,7 +27,7 @@ But there are a few additional edge cases that it supports. ### Clean the database ```sh - make initialize INIT_CLEAN=true + make initialize CLEAN=true ``` This will force the database to be recreated, and re-initialized. @@ -35,10 +35,10 @@ But there are a few additional edge cases that it supports. ### Load a data backup ```sh - make initialize [INIT_LOAD=] + make initialize [LOAD=] ``` - This command will load a data backup from a specified path. The optional `INIT_LOAD` argument allows you to + This command will load a data backup from a specified path. The optional `LOAD` argument allows you to specify the path to the data backup file. If not specified, the initialize command will determine if data should be loaded based on the current state of the databse, and will load the `_init` data backup. diff --git a/scripts/setup.py b/scripts/setup.py index 176a05237c51..8ee52a86f1ca 100755 --- a/scripts/setup.py +++ b/scripts/setup.py @@ -11,27 +11,38 @@ def set_env_file(values): print(f'{key}={value}') -def get_env_file(): +def get_env_file(path='.env'): env = {} - if os.path.exists('.env'): - with open('.env', 'r') as f: + if os.path.exists(path): + with open(path, 'r') as f: for line in f: key, value = line.strip().split('=', 1) - env[key] = value.strip('"') + if value.startswith('"') and value.endswith('"'): + value = value[1:-1] + env[key] = value return env -def get_value(key, default_value): - if key in os.environ: - return os.environ[key] +def get_value(key, default_value, from_file=True): + value = default_value + # Try to read the value from the .env file + # if we are allowed to do so + if from_file: + env = get_env_file() + if key in env: + value = env[key] - from_file = get_env_file() + # Always allow environment to override + # the default value + if key in os.environ: + value = os.environ[key] - if key in from_file: - return from_file[key] + # Do not allow empty or None values + if value is not None and value != '': + return value - return default_value + raise ValueError(f'{key} is not set') def get_docker_tag(): diff --git a/src/olympia/core/apps.py b/src/olympia/core/apps.py index 879b30868248..30083809d66a 100644 --- a/src/olympia/core/apps.py +++ b/src/olympia/core/apps.py @@ -53,12 +53,22 @@ def host_check(app_configs, **kwargs): actual_uid = getpwnam('olympia').pw_uid if actual_uid != expected_uid: - return [ + errors.append( Error( f'Expected user uid to be {expected_uid}, received {actual_uid}', id='setup.E002', ) - ] + ) + + if ( + settings.HOST_MOUNT is None or settings.HOST_MOUNT == 'production' + ) and os.path.exists('/data/olympia/Makefile-os'): + errors.append( + Error( + 'Makefile-os should be excluded by dockerignore', + id='setup.E003', + ) + ) return errors diff --git a/tests/make/test_setup.py b/tests/make/test_setup.py index 6656f64d66e3..c8a2772432b9 100644 --- a/tests/make/test_setup.py +++ b/tests/make/test_setup.py @@ -19,8 +19,10 @@ class BaseTestClass(unittest.TestCase): def assert_set_env_file_called_with(self, **kwargs): - expected = {key: kwargs.get(key, mock.ANY) for key in keys} - assert mock.call(expected) in self.mock_set_env_file.call_args_list + actual = self.mock_set_env_file.call_args_list[0].args[0] + for key in kwargs: + assert key in actual.keys() + assert actual.get(key) == kwargs.get(key, mock.ANY) def setUp(self): patch = mock.patch('scripts.setup.set_env_file') @@ -32,6 +34,101 @@ def setUp(self): self.mock_get_env_file = patch_two.start() +class TestBaseTestClass(BaseTestClass): + def test_invalid_key_raises(self): + with self.assertRaises(AssertionError): + main() + self.assert_set_env_file_called_with(FOO=True) + + +@override_env() +class TestGetEnvFile(BaseTestClass): + def setUp(self): + import tempfile + + self.tmp_dir = tempfile.mkdtemp() + self.path = os.path.join(self.tmp_dir, 'test_env') + + def tearDown(self): + import shutil + + shutil.rmtree(self.tmp_dir) + + def _write_and_assert(self, value, expected, write_value=True): + if write_value: + with open(self.path, 'w') as f: + f.write(f'key="{value}"') + expected_value = {'key': expected} if type(expected) is str else expected + self.assertEqual(get_env_file(self.path), expected_value) + + def test_get_env_file_missing(self): + self._write_and_assert('value', {}, write_value=False) + + def test_get_value_default(self): + self._write_and_assert('value', 'value') + + def test_get_empty_value(self): + self._write_and_assert('', '') + + def test_get_quoted_value(self): + self._write_and_assert('"quoted_value"', '"quoted_value"') + + def test_get_single_quoted_value(self): + self._write_and_assert("'quoted_value'", "'quoted_value'") + + def test_get_unmatched_quotes(self): + self._write_and_assert('"unmatched_quote', '"unmatched_quote') + + def test_get_nested_quotes(self): + self._write_and_assert( + 'value with "nested" quotes', 'value with "nested" quotes' + ) + + +class TestGetValue(BaseTestClass): + @override_env() + def test_get_value_from_default(self): + self.mock_get_env_file.return_value = {} + self.assertEqual(get_value('TEST_KEY', 'default'), 'default') + + @override_env() + def test_get_value_from_default_with_invalid_default(self): + self.mock_get_env_file.return_value = {} + for value in [None, '']: + with self.assertRaises(ValueError): + get_value('TEST_KEY', value) + + @override_env() + def test_get_value_from_file(self): + self.mock_get_env_file.return_value = {'TEST_KEY': 'file_value'} + self.assertEqual(get_value('TEST_KEY', 'default'), 'file_value') + + @override_env() + def test_get_value_from_file_invalid_value(self): + for value in [None, '']: + self.mock_get_env_file.return_value = {'TEST_KEY': value} + with self.assertRaises(ValueError): + get_value('TEST_KEY', value) + + @override_env(TEST_KEY='env_value') + def test_get_value_from_env(self): + self.assertEqual(get_value('TEST_KEY', 'default'), 'env_value') + + @override_env(TEST_KEY='env_value') + def test_get_value_from_env_overrides_file(self): + self.mock_get_env_file.return_value = {'TEST_KEY': 'file_value'} + self.assertEqual(get_value('TEST_KEY', 'default'), 'env_value') + + def test_get_value_from_env_invalid_value(self): + with override_env(TEST_KEY=''): + with self.assertRaises(ValueError): + get_value('TEST_KEY', None) + + with override_env(): + with self.assertRaises(ValueError): + get_value('TEST_KEY', None) + + @override_env() class TestGetDockerTag(BaseTestClass): def test_default_value_is_local(self):