feat: add php-hyperf image with otel variants#2
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughIntroduces the complete ChangesHyperf PHP 8.3 Image and CI/CD
Sequence Diagram(s)sequenceDiagram
actor Developer
participant GitHub as GitHub Actions
participant detect as detect job
participant build as build job (matrix)
participant DockerHub as Docker Hub
Developer->>GitHub: push to main (or workflow_dispatch with target)
GitHub->>detect: run detect job
detect->>detect: resolve */latest symlinks
detect->>detect: filter dirs containing Dockerfile
detect->>detect: parse variants.yaml → build descriptors JSON
detect-->>build: pass builds JSON array
build->>build: docker/setup-buildx-action
build->>DockerHub: docker login (DOCKERHUB_USERNAME/TOKEN)
build->>DockerHub: push devitools/<image>:<tag> linux/amd64
sequenceDiagram
participant Container as Container Start
participant entrypoint as entrypoint.sh
participant pgbouncer as pgbouncer
participant supervisord as supervisord
participant hyperf as hyperf app
participant otelcol as otelcol-contrib
Container->>entrypoint: exec /entrypoint.sh
alt PGBOUNCER_ENABLED != true
entrypoint->>supervisord: exec supervisord
else PGBOUNCER_ENABLED == true
entrypoint->>entrypoint: resolve PGBOUNCER_DATABASES aliases
entrypoint->>entrypoint: write /etc/pgbouncer/pgbouncer.ini
entrypoint->>entrypoint: write /etc/supervisor.d/pgbouncer.ini
entrypoint->>entrypoint: export pgb_<alias> env vars → 127.0.0.1:6432
entrypoint->>supervisord: exec supervisord
supervisord->>pgbouncer: start pgbouncer (via drop-in)
end
supervisord->>hyperf: start php /opt/www/bin/hyperf.php (priority 1)
supervisord->>otelcol: start otelcol-contrib (priority 2)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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 @.github/workflows/build-images.yml:
- Around line 1-17: The workflow is missing an explicit permissions block which
causes it to inherit broad repository defaults; add a top-level permissions
entry requesting the minimum required for this job (e.g. set permissions:
contents: read) so the GITHUB_TOKEN used by actions only has read access to the
repo; place this permissions block at the top-level (alongside keys like name
and on) so the workflow explicitly limits the GITHUB_TOKEN scope.
- Around line 40-45: The workflow currently leaves candidates empty when
triggered by workflow_dispatch without inputs.target because the
tj-actions/changed-files step is skipped; update the dispatch path in the
build-images job to either require inputs.target or fall back to a real diff
source: when github.event_name == "workflow_dispatch" and inputs.target is
empty, call or reuse the changed-files logic
(steps.changed.outputs.all_changed_files) or compute a diff against the default
branch (e.g., fetch origin and diff HEAD...origin/main) and populate candidates
from that output; ensure the code paths that reference inputs.target,
steps.changed.outputs.all_changed_files and candidates are adjusted so
candidates is non-empty for manual runs without a target.
- Around line 23-24: Update every GitHub Action `uses:` in the workflow to pin
to an immutable commit SHA instead of a moving tag: replace actions/checkout@v4,
tj-actions/changed-files@v44, docker/setup-buildx-action@v3,
docker/login-action@v3, and docker/build-push-action@v6 with their corresponding
OWNER/REPO@<full_sha> values (you can look up each repo’s latest stable commit
SHA) and optionally retain the human-readable tag as a trailing comment; ensure
the changes are made where those `uses:` entries appear so the workflow
references specific commit SHAs.
- Around line 37-43: The workflow is vulnerable to command injection by
interpolating GitHub expressions directly into shell blocks (candidates+=("${{
inputs.target }}"), image="${{ matrix.target }}", and for dir in ${{
steps.changed.outputs.all_changed_files }}); fix by passing these values through
the environment (use env: or GITHUB_OUTPUT to set a safe variable) and then
reading the environment vars inside the run block, validate the target/matrix
value against a strict allowlist before using it (e.g., check in a switch/if
against allowed names in the run step), and iterate changed files from a
pre-sanitized newline-separated env variable (or read from a file) rather than
unquoted expansion; update the blocks that reference candidates, inputs.target,
matrix.target, steps.changed.outputs.all_changed_files and the image assignment
to use the sanitized env vars and allowlist checks.
In `@hyperf-otel/8.3/Dockerfile`:
- Around line 13-20: Create and use an unprivileged service user in the
Dockerfile: add a non-root user (e.g., "hyperf") and group, chown the runtime
directories created by the RUN mkdir -p line (/etc/pgbouncer,
/var/log/pgbouncer, /var/run/pgbouncer, /etc/supervisor.d, /var/run/supervisor)
plus copied config files (COPY rootfs/... entries) to that user, and set USER to
that account before the ENTRYPOINT so /entrypoint.sh, otelcol-contrib, Hyperf
and optional pgbouncer run unprivileged; ensure the entrypoint still has execute
permission and any startup steps that need root (if any) are performed before
switching to the non-root user.
- Around line 7-12: The Dockerfile currently downloads
otelcol-contrib_${OTEL_COLLECTOR_VERSION}_linux_amd64.tar.gz and extracts it
without verifying integrity; update the RUN step that references
OTEL_COLLECTOR_VERSION and otelcol-contrib to fetch the release checksum (and/or
signature) from the same GitHub release, install any needed tools (sha256sum or
gnupg), verify the tarball against the published SHA256 (or verify the GPG
signature with the collector project's public key), fail the build if
verification fails, and only then extract to /usr/local/bin and remove
artifacts; ensure verification commands are added before the tar -xzf invocation
so the build aborts on mismatch.
In `@hyperf-otel/8.3/rootfs/entrypoint.sh`:
- Around line 35-41: The script writes a pgbouncer stanza even when resolved
backend variables (host, name, user, pass) are empty; change entrypoint.sh to
validate the per-alias variables (host, name, user, pass — allow port default)
before appending the stanza: if any required variable is empty, print a clear
error mentioning the alias to stderr and exit 1 instead of writing "pgb_${alias}
= ..." ; only append the stanza when the check passes. Use the existing variable
names (host, port, name, user, pass, alias) to locate and modify the block.
- Around line 18-23: The code builds a prefix from each PGBOUNCER_DATABASES
alias and passes it into eval (see resolve_prefix, the eval uses around "printf
... \"\${${prefix}_...:-}\"" and "export ${prefix}_...=..."), which is a
command-injection risk; fix by first validating each alias against a strict
whitelist (e.g., /^[A-Za-z0-9_]+$/) and aborting on any mismatch, then eliminate
eval by using safe environment lookups: construct the variable name (e.g.,
var="${prefix}_HOST") and retrieve its value with a non-eval method (use
printenv/printenv "$var" or, in bash, indirect expansion ${!var}) and assign
exports with safe shell expansions instead of eval; update resolve_prefix to
only produce validated uppercase prefixes and ensure all places that previously
used eval now use the validated prefix + safe env lookup and explicit export
commands.
In `@hyperf-otel/latest/Dockerfile`:
- Around line 13-20: Create a non-root service user and switch to it before
ENTRYPOINT: add a user (e.g., "hyperf" or "service") in the Dockerfile, chown
the runtime dirs and config/entrypoint files copied from rootfs (referencing
/etc/pgbouncer, /var/log/pgbouncer, /var/run/pgbouncer, /etc/supervisor.d,
/var/run/supervisor, /etc/otel-collector-config.yaml and /entrypoint.sh) so the
unprivileged user can write them at runtime, then set USER to that account
before the existing ENTRYPOINT ["/entrypoint.sh"]; if any initialization in
/entrypoint.sh requires root, perform that chown/setup at build time or wrap
only that portion with a drop-to-root helper, otherwise ensure the container
runs the otelcol-contrib, Hyperf and optional pgbouncer processes as the new
user.
- Around line 7-12: The Dockerfile currently downloads /tmp/otelcol.tar.gz using
OTEL_COLLECTOR_VERSION and extracts it without verification; update the RUN
sequence to fetch the published checksum (or signature) for the same release
(e.g. the SHA256 or GPG signature from the GitHub Releases for
v${OTEL_COLLECTOR_VERSION}), verify /tmp/otelcol.tar.gz before running tar -xzf
(fail the build if verification fails), and only then chmod and install
otelcol-contrib; reference the OTEL_COLLECTOR_VERSION variable,
/tmp/otelcol.tar.gz, and the otelcol-contrib binary when implementing the
verification step.
In `@hyperf-otel/latest/README.md`:
- Line 50: Replace the image tag used in the README examples: find every
occurrence of the string "devitools/hyperf-otel:8.3" (used in the Dockerfile
example lines like the FROM directive) and change it to
"devitools/hyperf-otel:latest" so the `latest` README shows the unversioned tag
in all examples.
In `@hyperf-otel/latest/rootfs/entrypoint.sh`:
- Around line 35-41: The code currently always writes a pgb_${alias} stanza even
if resolved vars (host, port, name, user, pass) are empty; update the block that
computes host/port/name/user/pass (using prefix and alias) to validate required
fields (at minimum host, name, user, pass) before appending to
/etc/pgbouncer/pgbouncer.ini: if any required variable is empty, print a clear
error naming the alias and missing fields and exit non‑zero instead of writing
the stanza; otherwise write the configured stanza as before. Ensure you
reference the same variables (host, port, name, user, pass, prefix, alias) and
fail fast when validation fails.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d22868b-adfd-4d97-b3d2-8c645c7964d6
📒 Files selected for processing (11)
.github/workflows/build-images.ymlhyperf-otel/8.3/Dockerfilehyperf-otel/8.3/README.mdhyperf-otel/8.3/rootfs/entrypoint.shhyperf-otel/8.3/rootfs/etc/otel-collector-config.yamlhyperf-otel/8.3/rootfs/etc/supervisord.confhyperf-otel/latest/Dockerfilehyperf-otel/latest/README.mdhyperf-otel/latest/rootfs/entrypoint.shhyperf-otel/latest/rootfs/etc/otel-collector-config.yamlhyperf-otel/latest/rootfs/etc/supervisord.conf
a215cbc to
458b51b
Compare
Restructures hyperf/ to follow the {image}/{version} pattern used by
aws-cli, node, quasar, vue and merges the standalone hyperf-otel image
into hyperf/8.3 as a multi-stage variant.
Targets:
- hyperf -> devitools/hyperf:8.3 (lean, ~97MB, ENTRYPOINT=php)
- hyperf-otel -> devitools/hyperf:8.3-<col> (~463MB, ENTRYPOINT=supervisord)
Collectors live in hyperf/8.3/otel/collectors/*.yaml. New variants can
be added by dropping a yaml file. Two ship in this PR:
- debug (vendor-neutral, logs spans to stderr)
- google (googlecloud exporter, replaces previous gcloud-specific default)
hyperf/latest is a symlink to 8.3.
Adds .github/workflows/build-images.yml: detects changed {image}/{version}
folders on push to main, follows latest symlinks, and publishes one tag
per (version, target) combo to Docker Hub via DOCKERHUB_USERNAME and
DOCKERHUB_TOKEN secrets.
458b51b to
b7d9de4
Compare
- workflows: declare explicit contents:read permissions and pass
GitHub-context values through env: instead of inline ${{ }}
expansion (mitigates template injection risk)
- workflow_dispatch: require target input (avoids empty matrix on
manual runs without input)
- hyperf/8.3/Dockerfile: verify otelcol-contrib tarball sha256
against the published opentelemetry-collector-releases checksums
- hyperf/8.3/otel/entrypoint.sh: whitelist PGBOUNCER_DATABASES
alias chars before eval, and fail fast when a required
POSTGRES_DB_* var is empty for an alias
Keep legacy hyperf/ untouched so devitools/hyperf is not overwritten. New multi-variant work (otel, debug, google, dev) lives under php-hyperf/8.3 and publishes to devitools/php-hyperf.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
php-hyperf/8.3/README-pt-BR.md (1)
35-60: ⚖️ Poor tradeoffDocumentation omits OTEL variants; should reflect the four published image variants.
The PR introduces four build variants—base, dev, otel-debug, and otel-google—but this README only documents dev vs. production. Users will be unaware that OTEL-instrumented variants are available for distributed tracing. Consider adding a section or expanding the comparison table to list all four variants and their use cases.
🤖 Prompt for 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. In `@php-hyperf/8.3/README-pt-BR.md` around lines 35 - 60, The documentation in the "Diferenças entre as Versões" section and comparison table only describes two image variants (dev and production) but the PR introduces four variants: base, dev, otel-debug, and otel-google. Expand the comparison table to include all four variants with their respective descriptions and use cases, ensuring that users are aware of the OTEL-instrumented variants available for distributed tracing and their specific purposes (otel-debug for debugging and otel-google for Google Cloud integration).
🤖 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 @.github/workflows/build-images.yml:
- Around line 115-133: The manifest schema validation using yq eval is missing
type checking for the 'suffix' field. Currently it only validates that the
'suffix' key exists with has("suffix"), but does not verify that the value is
actually a string type. This causes non-string values like numbers or objects to
pass validation and later fail during tag generation at the string concatenation
step with (if (.suffix // "") == "" then "" else "-" + .suffix end). Update the
yq validation condition to additionally check that 'suffix' is a string type (or
null/empty) before allowing the entry, ensuring type safety before the tag
generation logic uses it for string operations.
In `@php-hyperf/8.3/.scripts/setup-dev.sh`:
- Around line 34-39: The curl download of the sonar-scanner archive at lines
34-35 lacks integrity verification before extraction, creating a security risk.
After the initial curl command downloads sonar-scanner.zip, add a step to
download the corresponding .sha256 checksum file from binaries.sonarsource.com
and verify the archive using sha256sum -c before proceeding to the unzip
command. Additionally, consider downloading the .asc signature file and
verifying it using gpg against the SonarSource public key with fingerprint
679F1EE92B19609DE816FDE81DB198F93525EC1A to ensure authenticity. Insert these
verification steps between the curl download and the unzip extraction.
In `@php-hyperf/8.3/.scripts/setup.sh`:
- Around line 3-18: The TIMEZONE variable is not validated before being used to
create the symlink and PHP configuration. Add validation after the TIMEZONE
variable is set (after line 3) to check if the zoneinfo file exists at
/usr/share/zoneinfo/${TIMEZONE}. If the file does not exist, output an error
message and exit the script with a non-zero status code to fail fast, preventing
the creation of broken symlinks and invalid PHP timezone configuration.
In `@php-hyperf/8.3/README-pt-BR.md`:
- Line 42: The Docker Hub image name references in the README are using the
incorrect pattern `devitools/php-hyperf` when they should follow the correct
pattern `devitools/hyperf` as defined in the PR objectives. Find and replace all
occurrences of `devitools/php-hyperf` with `devitools/hyperf` throughout the
documentation, including in build commands (like the docker build command at
line 42), image name tables, and usage examples (which appear at lines 49, 58,
59, 67, and 73 according to the comment). This ensures users will pull the
correct Docker image from Docker Hub.
---
Nitpick comments:
In `@php-hyperf/8.3/README-pt-BR.md`:
- Around line 35-60: The documentation in the "Diferenças entre as Versões"
section and comparison table only describes two image variants (dev and
production) but the PR introduces four variants: base, dev, otel-debug, and
otel-google. Expand the comparison table to include all four variants with their
respective descriptions and use cases, ensuring that users are aware of the
OTEL-instrumented variants available for distributed tracing and their specific
purposes (otel-debug for debugging and otel-google for Google Cloud
integration).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45a0d3da-a7a1-48ec-a1aa-2e9dda887ae4
📒 Files selected for processing (16)
.github/workflows/build-images.yml.github/workflows/validate.ymlphp-hyperf/8.3/.scripts/setup-dev.shphp-hyperf/8.3/.scripts/setup.shphp-hyperf/8.3/Dockerfilephp-hyperf/8.3/README-pt-BR.mdphp-hyperf/8.3/README.mdphp-hyperf/8.3/otel/collectors/debug.yamlphp-hyperf/8.3/otel/collectors/google.yamlphp-hyperf/8.3/otel/entrypoint.shphp-hyperf/8.3/otel/supervisord.confphp-hyperf/8.3/rootfs/apk/repositoriesphp-hyperf/8.3/rootfs/etc/php/php-fpm.confphp-hyperf/8.3/rootfs/etc/php/php.iniphp-hyperf/8.3/variants.yamlphp-hyperf/latest
✅ Files skipped from review due to trivial changes (2)
- php-hyperf/latest
- php-hyperf/8.3/README.md
- workflows: tighten variants.yaml schema check (suffix must be string/null) - setup.sh: fail fast on invalid TIMEZONE and add tzdata so /etc/localtime resolves to a real zoneinfo file instead of a broken symlink - setup-dev.sh: verify sonar-scanner archive against its .sha256 before unzip - READMEs: document all four published variants (lean, dev, otel, google)
Summary
devitools/php-hyperfunderphp-hyperf/8.3/with multiple variants (lean, dev, otel-debug, otel-google) built from a single multi-stageDockerfile.hyperf/folder is left untouched —devitools/hyperfon Docker Hub is not overwritten by this PR.{image}/{version}layout (matchesaws-cli,node,quasar,vue);php-hyperf/latestis a symlink to8.3.variants.yamlmanifest per{image}/{version}that lists which tags to build (target + suffix + optional build-args).build-images.yml: on push tomain, readsvariants.yamlof changed{image}/{version}folders (followslatestsymlinks) and publishes one Docker Hub tag per entry.validate.yml: onpull_request, structurally validates every changedvariants.yaml.Why
quoteguide-functionsrepeatedly downloadotelcol-contrib(~80MB) and installsupervisor/pgbounceron every build. Pre-baking these into a base image saves ~15–25s per consumer build.googlecloud-specific; makingCOLLECTORa build-arg with multiple published variants lets consumers pick the right one atFROMtime.devitools/php-hyperf) avoids any risk of breaking currentdevitools/hyperfconsumers while the new image is validated in production.Variant manifest format
Rules:
target(non-empty) andsuffix(key required; empty value = plain:<version>tag).args(optional) is a mapping passed as--build-arg KEY=VALUE.<image>:<version>if suffix empty, else<image>:<version>-<suffix>.variants.yamlget a single plain build. Backwards-compatible withaws-cli,node, etc.Tags produced from this PR
hyperfdevitools/php-hyperf:8.3hyperfAPP_TARGET=devdevitools/php-hyperf:8.3-devhyperf-otelCOLLECTOR=debugdevitools/php-hyperf:8.3-otelhyperf-otelCOLLECTOR=googledevitools/php-hyperf:8.3-googlePlus matching
:latest,:latest-dev,:latest-otel,:latest-googlevia thephp-hyperf/latestsymlink (workflow follows it and includes it in the matrix when the canonical version changes).Workflows
build-images.yml(push tomain)*/*/Dockerfile,*/*/variants.yaml,*/*/rootfs/**,*/*/.scripts/**,*/*/otel/**, or*/latest.*/*/...glob means the legacyhyperf/(depth 1) is intentionally not picked up —devitools/hyperfstays frozen.{image}/{version}dir, follows anylatestsymlink pointing to it.(image, version): readsvariants.yamlviayq → jq(pre-installed on Ubuntu runners) and emits one matrix entry per variant.docker/build-push-action@v6call (linux/amd64) withtargetandbuild-argsderived from the manifest. GHA cache scoped per tag.workflow_dispatchwith a requiredtargetinput (e.g.php-hyperf/8.3).validate.yml(PRs)*/*/variants.yaml.yq).targetand thesuffixkey (value may be empty).argsis a mapping when present.DockerfiledeclaresFROM ... AS <target>for every referenced target.Security hardening (post-CodeRabbit review)
permissions: contents: readat top level.inputs.target,github.event_name, changed file lists) are passed throughenv:and validated against a[A-Za-z0-9_./-]allowlist before any shell expansion (mitigates template injection).otelcol-contribtarball is verified against the upstreamopentelemetry-collector-releases_otelcol-contrib_checksums.txtviasha256sum -cbefore extraction.PGBOUNCER_DATABASESalias against[A-Za-z0-9_]before using it ineval, and fails fast with a clear error when any requiredPOSTGRES_DB_*_HOST/NAME/USERNAME/PASSWORDenv var is empty.Adding new variants
Drop a row in
<image>/<version>/variants.yaml. The build workflow picks it up on the next push to main; the validate workflow catches malformed entries during the PR.Examples:
{ target: hyperf-otel, suffix: jaeger, args: { COLLECTOR: jaeger } }(with matchingphp-hyperf/8.3/otel/collectors/jaeger.yaml).{ target: hyperf, suffix: arm, args: { TARGETPLATFORM: linux/arm64 } }.Local validation
docker buildwith the appropriate--targetand--build-arg.:8.3has nootelcol-contrib/supervisordand keeps thephp /opt/www/bin/hyperf.php startentrypoint.:8.3-oteland:8.3-googlestartsupervisord(hyperf + otelcol-contrib + optional pgbouncer).validate.ymllogic smoke-tested locally: catches missing target/suffix, invalidargstype, missing Dockerfile stage, and duplicate suffix tags.Required secrets
DOCKERHUB_USERNAME✅ configuredDOCKERHUB_TOKEN✅ configuredTest plan
validate.yml; passes on this branch (manifest is valid).build-images.yml.php-hyperf/8.3× 4 variants +php-hyperf/latest× 4 variants).devitools/php-hyperf.:8.3and:latest,:8.3-devand:latest-dev, etc.docker pull devitools/php-hyperf:8.3(lean) keeps thephpentrypoint.docker pull devitools/php-hyperf:8.3-otelruns/entrypoint.sh(supervisord).devitools/hyperf:*tags on Docker Hub remain untouched (no workflow path matches the legacyhyperf/folder).Summary by CodeRabbit
New Features
Documentation