[#1607] Optimised FE build in Docker. #1804
Conversation
|
""" WalkthroughThe changes update Docker and Docker Compose configurations to standardize the handling of the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Docker as Docker Build
participant Theme as Theme Build Step
Dev->>Docker: Build CLI image (with DRUPAL_THEME)
alt DRUPAL_THEME is set
Docker->>Theme: Install Node dependencies & build assets
Theme-->>Docker: Assets built
else DRUPAL_THEME is empty
Docker-->>Docker: Skip theme build steps
end
Docker-->>Dev: CLI image ready
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (7)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings.ahoy.yml (1).docker/cli.dockerfile (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🔇 Additional comments (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
docker-compose.yml (1)
95-101: Quote build-arg to avoid YAML edge-cases
DRUPAL_THEMEis the only build arg that is not wrapped in quotes.
Although most parsers will handle it, quoting is safer when the value can contain special characters (spaces, colons,#, etc.) and keeps the style consistent with the other args.- DRUPAL_THEME: ${DRUPAL_THEME:-} + DRUPAL_THEME: "${DRUPAL_THEME:-}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (11)
.vortex/installer/tests/Fixtures/install/_baseline/.docker/cli.dockerfileis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_lagoon/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/.docker/cli.dockerfileis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_database_lagoon/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_clamav/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_solr/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_valkey/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.docker/cli.dockerfileis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_custom/.docker/cli.dockerfileis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (6)
.docker/cli.dockerfile(2 hunks).vortex/tests/bats/fixtures/docker-compose.env.json(1 hunks).vortex/tests/bats/fixtures/docker-compose.env_local.json(1 hunks).vortex/tests/bats/fixtures/docker-compose.env_mod.json(1 hunks).vortex/tests/bats/fixtures/docker-compose.noenv.json(1 hunks)docker-compose.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Docker images should be updated for container base images
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.docker/cli.dockerfile (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
🔇 Additional comments (4)
.vortex/tests/bats/fixtures/docker-compose.env_mod.json (1)
30-37: Theme variable added consistently – looks goodThe fixture now mirrors the compose file and Dockerfile logic by injecting
DRUPAL_THEMEin both build args and runtime envs. No issues spotted.Also applies to: 90-97, 118-126
.vortex/tests/bats/fixtures/docker-compose.env.json (1)
30-37: Fixtures aligned with newDRUPAL_THEMEhandlingAll affected services include the variable, which will let the conditional build steps run during tests. LGTM.
Also applies to: 90-97, 118-126
.vortex/tests/bats/fixtures/docker-compose.env_local.json (1)
30-37: Local-env fixture kept in syncNo functional concerns; addition keeps local fixture behaviour in line with others.
Also applies to: 90-97, 118-126
.vortex/tests/bats/fixtures/docker-compose.noenv.json (1)
35-36: Edge-case coverage: empty theme valueNice touch covering the “theme not set” path. Please make sure the CLI Dockerfile logic treats an empty string the same as an unset var (current
ARG DRUPAL_THEME=""+if [ -n "$DRUPAL_THEME" ]pattern is fine).
No code changes required.Also applies to: 123-124
| ARG DRUPAL_THEME="" | ||
| ENV DRUPAL_THEME=${DRUPAL_THEME} | ||
| #;> DRUPAL_THEME |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Defaulting DRUPAL_THEME to an empty string can mask mis-configuration
With an empty default the build silently skips the front-end step when the variable is forgotten.
Consider leaving the ARG unset (no default) and/or adding a validation guard so that a typo is surfaced early.
🤖 Prompt for AI Agents
In .docker/cli.dockerfile around lines 35 to 37, the ARG DRUPAL_THEME is
defaulted to an empty string, which can hide misconfigurations by silently
skipping the front-end step if the variable is forgotten. Remove the default
empty string from the ARG declaration so it is unset by default, and add a
validation step in the Dockerfile to check if DRUPAL_THEME is set, failing the
build early with a clear error message if it is missing or empty.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1804 +/- ##
========================================
Coverage 72.68% 72.68%
========================================
Files 84 84
Lines 4711 4711
Branches 35 35
========================================
Hits 3424 3424
Misses 1287 1287 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/1607-conditional-fe-build
branch
from
d68ce53 to
db1e386
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
docker-compose.yml (1)
95-101: Build-arg addition looks good; ensure.envdocs are updatedPassing
DRUPAL_THEMEexplicitly to the CLI build solves the “ARG not defined” warning introduced in the Dockerfile refactor. 👍
Double-check that the variable is documented in your.env.example(or README) so newcomers know they must export it to trigger the FE build..vortex/tests/bats/fixtures/docker-compose.env_mod.json (1)
118-126: Fixture value duplicated in bothbuild.argsandenvironmentMirroring the variable in both blocks is fine but brittle – if a test ever changes the theme name you’ll have to edit two locations for every service.
Consider trimmingbuild.args.DRUPAL_THEMEand relying on the run-time env only; the CLI Dockerfile already falls back to the env when the ARG is missing.Also applies to: 132-149
.docker/cli.dockerfile (1)
77-91: Front-end build still breaks layer caching
COPY . /appprecedes the (heavy) Yarn install, so any code change invalidates the cache for Node dependencies. Copy only the theme’spackage.json/yarn.lockfirst, runyarn install, then copy the rest of the repo. This was flagged previously and remains unaddressed.
♻️ Duplicate comments (3)
.vortex/tests/bats/fixtures/docker-compose.env.json (1)
118-126: Same duplication pattern as other fixtureSame comment as above – drop one copy or add a helper in fixture generation to keep them in sync.
Also applies to: 132-149
.vortex/tests/bats/fixtures/docker-compose.env_local.json (1)
118-126: Local-env fixture duplicationSee earlier note on keeping ARG vs ENV in sync.
Also applies to: 132-149
.docker/cli.dockerfile (1)
34-37: LeavingDRUPAL_THEMEdefault empty still masks mis-configuration
The default""causes the step to be silently skipped when the arg is forgotten or misspelled. Either drop the default or fail early if it’s empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (11)
.vortex/installer/tests/Fixtures/install/_baseline/.docker/cli.dockerfileis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_lagoon/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/.docker/cli.dockerfileis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_database_lagoon/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_clamav/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_solr/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_valkey/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.docker/cli.dockerfileis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_custom/.docker/cli.dockerfileis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (6)
.docker/cli.dockerfile(2 hunks).vortex/tests/bats/fixtures/docker-compose.env.json(1 hunks).vortex/tests/bats/fixtures/docker-compose.env_local.json(1 hunks).vortex/tests/bats/fixtures/docker-compose.env_mod.json(1 hunks).vortex/tests/bats/fixtures/docker-compose.noenv.json(1 hunks)docker-compose.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Docker images should be updated for container base images
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.docker/cli.dockerfile (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Docker images should be updated for container base images
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-docs
🔇 Additional comments (1)
.vortex/tests/bats/fixtures/docker-compose.noenv.json (1)
118-126:noenvfixture: blank value correctly disables theme buildGood catch setting the ARG to an empty string here – matches the intended “no theme” scenario.
Also applies to: 132-149
| RUN if [ -n "${DRUPAL_THEME}" ]; then \ | ||
| theme_path="/app/${WEBROOT}/themes/custom/${DRUPAL_THEME}"; \ | ||
| yarn --cwd="${theme_path}" install --frozen-lockfile --no-progress && \ | ||
| yarn --cwd="${theme_path}" run build && \ | ||
| yarn cache clean; \ | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing theme directory & make Yarn fully non-interactive
If DRUPAL_THEME is set but the directory is absent the build will error mid-way; bail out explicitly. Also add --non-interactive to avoid hanging CI pipelines.
RUN if [ -n "${DRUPAL_THEME}" ]; then \
theme_path="/app/${WEBROOT}/themes/custom/${DRUPAL_THEME}"; \
+ if [ ! -d "${theme_path}" ]; then \
+ echo "Theme directory '${theme_path}' does not exist." >&2; exit 1; \
+ fi && \
- yarn --cwd="${theme_path}" install --frozen-lockfile --no-progress && \
+ yarn --cwd="${theme_path}" install --frozen-lockfile --non-interactive --no-progress && \
yarn --cwd="${theme_path}" run build && \
yarn cache clean; \
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN if [ -n "${DRUPAL_THEME}" ]; then \ | |
| theme_path="/app/${WEBROOT}/themes/custom/${DRUPAL_THEME}"; \ | |
| yarn --cwd="${theme_path}" install --frozen-lockfile --no-progress && \ | |
| yarn --cwd="${theme_path}" run build && \ | |
| yarn cache clean; \ | |
| fi | |
| RUN if [ -n "${DRUPAL_THEME}" ]; then \ | |
| theme_path="/app/${WEBROOT}/themes/custom/${DRUPAL_THEME}"; \ | |
| if [ ! -d "${theme_path}" ]; then \ | |
| echo "Theme directory '${theme_path}' does not exist." >&2; exit 1; \ | |
| fi && \ | |
| yarn --cwd="${theme_path}" install --frozen-lockfile --non-interactive --no-progress && \ | |
| yarn --cwd="${theme_path}" run build && \ | |
| yarn cache clean; \ | |
| fi |
🤖 Prompt for AI Agents
In .docker/cli.dockerfile around lines 86 to 91, the current script assumes the
theme directory exists if DRUPAL_THEME is set, which can cause errors if the
directory is missing. Modify the RUN command to first check if the theme
directory exists before running yarn commands, and add the --non-interactive
flag to all yarn commands to prevent hanging in CI environments.
db1e386 to
d2061e2
Compare
Related to #1607
Summary by CodeRabbit