feat: extract common base image #1078
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a modular build system for amp-devcontainer by extracting common dependencies into a reusable base image. The change aims to improve maintainability, reduce build times through image layering, and streamline CI/CD workflows.
Changes:
- Created a new
amp-devcontainer-baseimage with shared dependencies (BATS testing tools, certificates, common packages) - Refactored C++ and Rust Dockerfiles to build from the base image instead of duplicating setup logic
- Updated CI/CD workflows to build the base image first, then use its digest for flavor builds
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.devcontainer/base/Dockerfile |
Defines the new base image with common dependencies extracted from flavor-specific Dockerfiles |
.devcontainer/base/apt-requirements-base.json |
Centralizes base package versions previously duplicated across flavor files |
.devcontainer/base/devcontainer-metadata.json |
Provides metadata configuration for the base image |
.devcontainer/cpp/Dockerfile |
Refactored to use base image and removed duplicated setup steps |
.devcontainer/cpp/apt-requirements-base.json |
Cleaned up to only include C++-specific packages |
.devcontainer/cpp/devcontainer.json |
Added build configuration to use locally-built base image |
.devcontainer/rust/Dockerfile |
Refactored to use base image and removed duplicated setup steps |
.devcontainer/rust/apt-requirements-base.json |
Cleaned up to only include Rust-specific packages |
.devcontainer/rust/devcontainer.json |
Added build configuration to use locally-built base image |
.github/workflows/wc-build-push.yml |
Added support for build arguments and version output |
.github/workflows/wc-build-push-test.yml |
Reordered inputs alphabetically and added build-args support |
.github/workflows/continuous-integration.yml |
Added base image build job and wired outputs to flavor builds |
.github/workflows/release-build.yml |
Added base image build job and updated release notes generation |
.github/RELEASE_TEMPLATE.md |
Added base image to container table |
README.md |
Added documentation for the new base image |
| "unzip": "6.0-28ubuntu4.1", | ||
| "wget": "1.21.4-1ubuntu4.1", | ||
| "xsltproc": "1.1.39-0exp1ubuntu0.24.04.3", | ||
| "xz-utils": "5.6.1+really5.4.5-1ubuntu0.2" |
There was a problem hiding this comment.
The package xz-utils is now duplicated between the base image (apt-requirements-base.json) and the C++ flavor. Since it's already included in the base, it should be removed from this file to avoid duplication.
| --mount=type=cache,target=/var/lib/apt,sharing=locked \ | ||
| --mount=type=cache,target=/var/log,sharing=locked \ | ||
| apt-get update && apt-get install -y --no-install-recommends jq \ | ||
| apt-get update \ |
There was a problem hiding this comment.
The apt-get update command should be combined with the package installation on the same line using && to reduce layers and ensure the cache is not stale. Currently, if line 26 changes but line 25 doesn't, a cached layer with an outdated package index could be used.
| # Include the Cisco Umbrella PKI Root | ||
| && wget --no-hsts -qO /usr/local/share/ca-certificates/Cisco_Umbrella_Root_CA.crt https://www.cisco.com/security/pki/certs/ciscoumbrellaroot.pem \ | ||
| && update-ca-certificates \ | ||
| apt-get update \ |
There was a problem hiding this comment.
The apt-get update command should be combined with the package installation on the same line using && to reduce layers and ensure the cache is not stale. Currently, if line 42 changes but line 41 doesn't, a cached layer with an outdated package index could be used.
|
📦 Container Size AnalysisNote Comparing 📈 Size Comparison Table
|
✅MegaLinter analysis: Success
See detailed reports in MegaLinter artifacts Your project could benefit from a custom flavor, which would allow you to run only the linters you need, and thus improve runtime performances. (Skip this info by defining
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
[MegaLinter] reported by reviewdog 🐶
amp-devcontainer/.devcontainer/cpp/devcontainer.json
Lines 11 to 13 in be5f51c
📦 Container Size AnalysisNote Comparing 📈 Size Comparison Table
|
📦 Container Size AnalysisNote Comparing 📈 Size Comparison Table
|
Test Results 5 files ±0 5 suites ±0 5m 46s ⏱️ + 2m 22s For more details on these failures, see this check. Results for commit be5f51c. ± Comparison against base commit 5ec2927. |
|
|
||
| # Install the base system with all tool dependencies | ||
| # hadolint ignore=DL3008 | ||
| RUN --mount=type=bind,source=.devcontainer/base/apt-requirements-base.json,target=/tmp/apt-requirements-base.json \ |
There was a problem hiding this comment.
I never used this mount approach for RUNs. How does this play with build step caching, does a change in the input file still ensure the RUN is rerun?
| ARG INCLUDE_WHAT_YOU_USE_VERSION=0.23 | ||
| ARG XWIN_VERSION=0.6.7 | ||
|
|
||
| ARG AMP_DEVCONTAINER_VERSION=local |
There was a problem hiding this comment.
Isn't it safer to use unknown here as well?
| ARG CARGO_BINSTALL_VERSION=1.15.11 | ||
| ARG RUST_VERSION=1.91.1 | ||
|
|
||
| ARG AMP_DEVCONTAINER_VERSION=local |
| description: "Path to the Playwright acceptance tests (directory that contains playwright.config.ts)" | ||
| required: false | ||
| type: string | ||
| build-args: | ||
| description: "Optional docker build args (newline-separated KEY=VALUE)" |
There was a problem hiding this comment.
[nitpick] Quotes for description not required here. I see the file already had more of these so i'll leave it up to you
🚀 Hey, I have created a Pull Request
Description of changes
This pull request introduces a new modular build system for devcontainers by creating a reusable base image and updating the build pipelines, configuration, and Dockerfiles for both the C++ and Rust development containers. The main goals are to improve maintainability, speed up builds by leveraging a shared base image, and streamline CI/CD workflows.
The most important changes are:
1. Introduction of a Modular Base Image
.devcontainer/base/Dockerfile, which defines a newamp-devcontainer-baseimage with core dependencies, BATS testing tools, and common setup steps. This image is now used as the foundation for both C++ and Rust devcontainers. (.devcontainer/base/Dockerfile, .devcontainer/base/DockerfileR1-R55).devcontainer/base/apt-requirements-base.jsonand.devcontainer/base/devcontainer-metadata.jsonto manage base package versions and metadata centrally. [1] [2]2. Refactoring of C++ and Rust Devcontainer Dockerfiles
.devcontainer/cpp/Dockerfileand.devcontainer/rust/Dockerfileto use the new base image via theBASE_IMAGEbuild argument, removing duplicated setup logic and package installations now handled in the base. [1] [2]apt-requirements-base.jsonfiles to only include flavor-specific dependencies. [1] [2]3. CI/CD Pipeline and Workflow Enhancements
continuous-integration.yml,release-build.yml) to add a dedicated job for building and pushing the base image before building the flavor-specific images. The flavor builds now depend on outputs from the base build, ensuring correct versioning and image layering. [1] [2] [3] [4]4. Improved Local Development Experience
initializeCommandand build arguments to the flavordevcontainer.jsonfiles, ensuring the base image is built locally before building the flavor image and that the correct image versions are used. (.devcontainer/cpp/devcontainer.json,.devcontainer/rust/devcontainer.json, [1] [2]5. Documentation and Release Process Updates
.github/RELEASE_TEMPLATE.md,.github/workflows/release-build.yml, [1] [2]✔️ Checklist