"Fix makefile env variables definition" #22246
Conversation
KevinMind
force-pushed
the
fix-make-vars
branch
2 times, most recently
from
4559241
to
10a7605
Compare
KevinMind
force-pushed
the
fix-make-vars
branch
2 times, most recently
from
e6c3479
to
ef42735
Compare
|
|
||
| # Expect image tag is correct | ||
| echo $config | grep -q "image: mozilla/addons-server:${{ matrix.expected.version }}" | ||
| export SUPERUSER_EMAIL="email" |
There was a problem hiding this comment.
I could probably reimplement this in python... wdyt?
There was a problem hiding this comment.
Probably worth extracting somewhere (in Python or not) cause there is a lot going on and it's hard to read...
There was a problem hiding this comment.
I've extracted the exiisting bash test, and the new tests introduced here to a JS make.spec.js
The reason for JS is that the test MUST run on the host, it relies on docker commands and we don't (probably don't want to) install docker in our container. no need for the bloat.
Theoretically I could write it in python, but I felt it easier to run the js tests on the host.
.circleci/config.yml
Outdated
| @@ -597,6 +597,14 @@ jobs: | |||
| --ignore src/olympia/versions/ \ | |||
| --ignore src/olympia/zadmin | |||
|
|
|||
| make_tests: | |||
There was a problem hiding this comment.
This should run as a separate test because it modifies the environment and .env files... it's theoretically possible to clean up that data after, but probably not worth the inherent risk.
KevinMind
force-pushed
the
fix-make-vars
branch
2 times, most recently
from
cd678a2
to
c125046
Compare
| @@ -1,4 +1,9 @@ | |||
| x-env-mapping: &env | |||
| # https://docs.docker.com/compose/environment-variables/envvars-precedence/ | |||
| env_file: | |||
There was a problem hiding this comment.
This is an important, but I think good change. This means if there is no .env file, docker compose will just fail. It would prevent someone from running anything via docker compose before running make create_env_file.
There is not really a scenario where you should run docker compose without it, it is critical for configuring the compose project.
happy to hear push back if you think this is too strong enforcement.
There was a problem hiding this comment.
Boo, docker compose version in gha is not compatible with this syntax. Not sure it's worth the effort to upgrade.
There was a problem hiding this comment.
NVM. By default env_files are required. so we get the same behavior.
KevinMind
force-pushed
the
fix-make-vars
branch
4 times, most recently
from
78b2169
to
69ad228
Compare
Fixes: mozilla/addons#14787
Description
The makefile env variables were not defined correctly, causing issues with the configuration of the containers. This pull request fixes the definition of the makefile env variables to ensure proper configuration.
Context
We have 4 possible values of critical environment variables.
Critical environment variables are exactly those which might produce different behavior if running a command via a make target, or directly.
E.g. we have variables defined in our compose files which should be define on the environment. If docker compose is run via make, values will be managed appropriately, but unless the value is in the .env or specified in the environment running docker compose directly will use a different or no value.
Testing
I've added a CI job to verify that the make create_env_file will produce expected results given all 8 possible combinations of the 4 possible sources
You can also manually test
Remove .env file
run
Expect all values to be the value defined in the default parameter via make file.
Modify one of the values in the produced in the .env and rerun the command.
Expect the value is persisted from the .env
Keep the .env file, and set the value in the shell before running the command. Re-run and expect the env to take priority.
Now adding to this, if you further specify an argument and expect it to take precedence over all values, regardless of if env/file/default are specified.
Expect value to be arg now.
The CI check runs through effectively this set of steps for every variable that is in this critical list.