Embed build info into the docker image instead of relying on runtime variables. #22930
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KevinMind
force-pushed
the
addons-15066-build-info
branch
from
ea0dc7b to
68156b4
Compare
KevinMind
force-pushed
the
addons-15066-tests
branch
from
b619b89 to
ec052e8
Compare
KevinMind
force-pushed
the
addons-15066-build-info
branch
2 times, most recently
from
1bd77b3 to
96337d4
Compare
KevinMind
force-pushed
the
addons-15066-build-info
branch
from
96337d4 to
2da734e
Compare
KevinMind
changed the title
KevinMind
force-pushed
the
addons-15066-build-info
branch
18 times, most recently
from
6fd59d5 to
cff36e2
Compare
KevinMind
requested review from
a team and
eviljeff
and removed request for
a team
eviljeff
reviewed
Dec 13, 2024
Makefile-os
Outdated
Comment on lines
10
to
13
| # Values that are not saved to .env | ||
| # but should be set in the docker image | ||
| # default to static values to prevent | ||
| # invalidating docker build cache |
There was a problem hiding this comment.
Not intended, but now it is an actual haiku 👍
KevinMind
force-pushed
the
addons-15066-build-info
branch
from
cff36e2 to
3649368
Compare
KevinMind
force-pushed
the
addons-15066-build-info
branch
from
3649368 to
b64d766
Compare
eviljeff
approved these changes
Dec 13, 2024
| 'DOCKER_VERSION', | ||
| 'DOCKER_BUILD', | ||
| ]; | ||
| it('.services.(web|worker).environment excludes build info variables', () => { |
There was a problem hiding this comment.
I forgot to add this last time: I'm not clear what this is testing, exactly. What aren't we including in the web/worker envs, and why?
There was a problem hiding this comment.
We exclude variables that could be interpreted as build info such as the DOCKER_COMMIT, DOCKER_VERSION and (eventually) DOCKER_TARGET. You don't want to live in a world where you build with one commit/version and then run the container with another value set (arbitrarily) and then your container thinks it is a different image.. This test confirms that the variables that are used in the build info file are NOT available on the environment of the container.
KevinMind
force-pushed
the
addons-15066-build-info
branch
from
e0c4c07 to
f988d74
Compare
- Updated Dockerfile to create a build info file containing static build variables (commit, version, build, target) that are now read-only at runtime. - Modified get_version_json function to read build information from the newly created build info file instead of relying on environment variables. - Introduced REQUIRED_VERSION_KEYS to ensure all necessary keys are present in version.json during checks. - Enhanced tests to validate the presence of required version keys and ensure proper functionality of the get_version_json method. - Updated docker-bake.hcl to maintain consistency in build arguments. TMP: use json instead Update src/olympia/core/utils.py Co-authored-by: Andrew Williamson <[email protected]>
KevinMind
force-pushed
the
addons-15066-build-info
branch
from
f988d74 to
a094ce2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: mozilla/addons#15230
Description
Dockerfileto expose build arguments and create a static build info file, ensuring that build variables are hard-coded and not overridden at runtime.get_version_jsonfunction inutils.pyto read build information from the newly created static file, ensuring required keys are validated.test_apps.pyandtest_utils.pyto check for required version keys and validate the build info retrieval process.Context
Previously relying on runtime environment variables led to less consistent checks because runtime variables can change, where the underlying image cannot. Since we are mostly checking the underlying image, we should not rely on the runtime variables but instead the image itself.
Testing
Checklist
#ISSUENUMat the top of your PR to an existing open issue in the mozilla/addons repository.