Skip to content

[#2816] Fixed Docker build check warnings in the CLI Dockerfile.#2817

Merged
AlexSkrypnyk merged 4 commits into
mainfrom
feature/2816-build-warnings
Jul 21, 2026
Merged

[#2816] Fixed Docker build check warnings in the CLI Dockerfile.#2817
AlexSkrypnyk merged 4 commits into
mainfrom
feature/2816-build-warnings

Conversation

@AlexSkrypnyk

@AlexSkrypnyk AlexSkrypnyk commented Jul 21, 2026

Copy link
Copy Markdown
Member

Closes #2816

Summary

Lagoon (BuildKit) builds emitted Docker build-check warnings from .docker/cli.dockerfile: SecretsUsedInArgOrEnv on the PACKAGE_TOKEN argument and UndefinedVar on $PHP_INI_SCAN_DIR. Both are false positives. This change silences them at the source - the name-based secret check is skipped where the token is handled safely, and the undefined-variable check is fixed properly by declaring the argument. After the change, docker build --check reports zero warnings on .docker/cli.dockerfile.

Changes

.docker/cli.dockerfile

  • Added a # check=skip=SecretsUsedInArgOrEnv parser directive on the first line, with a header comment explaining why. The check fires purely on the PACKAGE_TOKEN argument name. The token is consumed through a --mount=type=secret build secret and is never written to an image layer (it is an ARG, not an ENV), so nothing sensitive is baked in. The argument cannot be removed (docker-compose.yml passes PACKAGE_TOKEN as a build arg) or renamed (it is a public Vortex variable), and Docker offers no per-instruction skip, so a file-level directive is the idiomatic suppression.
  • Declared ARG PHP_INI_SCAN_DIR="" immediately before the ENV that references it. This is Docker's documented fix for UndefinedVar (declare before use). It is behaviour-preserving: when the base image does not set the variable it stays empty and the leading colon still appends our directory to the default PHP INI scan directories. Fixing this properly (rather than also skipping the check) keeps UndefinedVar active for the rest of the file.

Installer fixtures

  • Regenerated the installer baseline snapshots so the generated consumer CLI Dockerfile matches the updated template.

Before / After

docker build --check -f .docker/cli.dockerfile .

BEFORE                                   AFTER
  WARNING SecretsUsedInArgOrEnv            Check complete,
          (ARG "PACKAGE_TOKEN")            no warnings found.
  WARNING UndefinedVar
          ($PHP_INI_SCAN_DIR)
  => 2 warnings found

…iles.

Skipped the name-based 'SecretsUsedInArgOrEnv' check on the containers where the package token and the local database password are handled safely, and declared 'PHP_INI_SCAN_DIR' as an argument to satisfy the 'UndefinedVar' check while preserving the leading-colon default-scan-dir behaviour.
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The Dockerfiles document intentional SecretsUsedInArgOrEnv skips. The CLI image defines PHP_INI_SCAN_DIR with an empty default before appending its Drush scan path, preventing undefined-variable warnings.

Changes

Docker build validation

Layer / File(s) Summary
Build checks and variable initialization
.docker/cli.dockerfile, .docker/database.dockerfile
The CLI Dockerfile documents secret-based PACKAGE_TOKEN handling and initializes PHP_INI_SCAN_DIR; the database Dockerfile documents its non-secret MYSQL_PASSWORD default and skips the secrets check.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

I’m a rabbit with a Dockerfile tune,
Hopping past warnings beneath the moon.
Secrets stay safely out of sight,
Scan paths now resolve just right.
Carrots for builds—what a delight!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The CLI Dockerfile fixes both reported warnings, and the database Dockerfile change matches the stated secondary objective.
Out of Scope Changes check ✅ Passed The database Dockerfile update is in scope because the PR objective explicitly includes suppressing the false-positive MYSQL_PASSWORD warning.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: fixing Docker build check warnings, though it only mentions the CLI Dockerfile.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/2816-build-warnings

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.docker/cli.dockerfile:
- Around line 1-9: Remove the PACKAGE_TOKEN ARG fallback and any related
environment usage from the CLI Dockerfile, including its package-install logic.
Update the corresponding docker-compose CLI build configuration to pass
PACKAGE_TOKEN exclusively through build secrets, removing it from build.args;
preserve the existing secret mount behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 62ec1ed4-ceed-4c45-8487-67b651e773e1

📥 Commits

Reviewing files that changed from the base of the PR and between 66aa626 and ac845df.

⛔ Files ignored due to path filters (13)
  • .vortex/installer/tests/Fixtures/handler_process/_baseline/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/_baseline/.docker/database.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/names/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/theme_claro/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/theme_custom/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/theme_olivero/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/theme_stark/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_groups_no_fe_lint_no_theme/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_groups_no_fe_lint_no_theme_circleci/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_no_eslint_no_theme/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_no_stylelint_no_theme/.docker/cli.dockerfile is excluded by !.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (2)
  • .docker/cli.dockerfile
  • .docker/database.dockerfile

Comment thread .docker/cli.dockerfile
@github-actions

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

2 similar comments
@AlexSkrypnyk

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.35%. Comparing base (66aa626) to head (c8ac27e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2817      +/-   ##
==========================================
- Coverage   86.77%   86.35%   -0.42%     
==========================================
  Files          98       91       -7     
  Lines        4778     4624     -154     
  Branches       47        3      -44     
==========================================
- Hits         4146     3993     -153     
+ Misses        632      631       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown

📖 Documentation preview for this pull request has been deployed to Netlify:

https://6a5f2db7b398819d5a57c884--vortex-docs.netlify.app

This preview is rebuilt on every commit and is not the production documentation site.

@AlexSkrypnyk AlexSkrypnyk added the Needs review Pull request needs a review from assigned developers label Jul 21, 2026
The database container runs as a Lagoon-managed 'mariadb' service and is built only for local and CI use, and its 'MYSQL_PASSWORD' default is a real value baked into the image rather than a name-only false positive, so it is left outside this change.
@AlexSkrypnyk AlexSkrypnyk changed the title [#2816] Fixed Docker build check warnings in CLI and database Dockerfiles. [#2816] Fixed Docker build check warnings in the CLI Dockerfile. Jul 21, 2026
@AlexSkrypnyk

This comment has been minimized.

@github-actions

Copy link
Copy Markdown

Code coverage (threshold: 90%)

  Classes: 100.00% (1/1)
  Methods: 100.00% (2/2)
  Lines:   98.55% (204/207)
Per-class coverage
Drupal\ys_demo\Plugin\Block\CounterBlock
  Methods: 100.00% ( 2/ 2)   Lines: 100.00% ( 10/ 10)

@AlexSkrypnyk

This comment has been minimized.

1 similar comment
@AlexSkrypnyk

Copy link
Copy Markdown
Member Author

Code coverage (threshold: 90%)

  Classes: 100.00% (1/1)
  Methods: 100.00% (2/2)
  Lines:   98.55% (204/207)
Per-class coverage
Drupal\ys_demo\Plugin\Block\CounterBlock
  Methods: 100.00% ( 2/ 2)   Lines: 100.00% ( 10/ 10)

@AlexSkrypnyk
AlexSkrypnyk enabled auto-merge (squash) July 21, 2026 08:30
@AlexSkrypnyk AlexSkrypnyk added the AUTOMERGE Pull request has been approved and set to automerge label Jul 21, 2026
@AlexSkrypnyk
AlexSkrypnyk merged commit 429d89a into main Jul 21, 2026
34 checks passed
@AlexSkrypnyk
AlexSkrypnyk deleted the feature/2816-build-warnings branch July 21, 2026 08:31
@github-project-automation github-project-automation Bot moved this from BACKLOG to Release queue in Vortex 1.x Jul 21, 2026
AlexSkrypnyk added a commit that referenced this pull request Jul 22, 2026
Forward-ported source from main 429d89a; installer fixtures regenerated via update-snapshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A3 Board worker 3 AUTOMERGE Pull request has been approved and set to automerge Needs review Pull request needs a review from assigned developers

Projects

Status: Release queue

Development

Successfully merging this pull request may close these issues.

In Lagoon, when CLI builds there are missing variables warning

1 participant