Skip to content

Conversation

@endigma
Copy link
Member

@endigma endigma commented Dec 9, 2025

Thank you @joornby-angel for your contribution!

Hijacked from #2242

Summary by CodeRabbit

  • Chores
    • Standardized container platform selection across services to improve multi-architecture compatibility and image consistency.
  • Bug Fixes
    • Updated collector build and startup configuration to produce more consistent runtime behavior and clearer startup handling.

✏️ Tip: You can customize this high-level summary in your review settings.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.

@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

Walkthrough

Multiple Dockerfiles switch platform selection from BUILDPLATFORM to TARGETPLATFORM in FROM instructions. otelcollector also changes its builder base image to golang:1.25, prefixes its ocb invocation with GOOS/GOARCH, and removes ENTRYPOINT/CMD. No exported/public API changes.

Changes

Cohort / File(s) Summary
Platform substitutions (single/final stage)
admission-server/Dockerfile, cdn-server/Dockerfile, controlplane/Dockerfile, graphqlmetrics/Dockerfile, studio/Dockerfile
Replaced --platform=${BUILDPLATFORM} / ${BUILDPLATFORM} with --platform=${TARGETPLATFORM} / ${TARGETPLATFORM} in one or more FROM stages; other build steps unchanged.
Platform substitutions (multi-stage)
router
router/Dockerfile
Replaced --platform=${BUILDPLATFORM} with --platform=${TARGETPLATFORM} in builder and runtime FROM stages; added WORKDIR / in final stage; other lines unchanged.
Platform substitution + EXPOSE unchanged
keycloak/Dockerfile
Replaced BUILDPLATFORM with TARGETPLATFORM in FROM; EXPOSE directives and ports preserved; trailing newline adjusted.
Builder changes, GO env for ocb, startup removed
otelcollector/Dockerfile
Builder FROM --platform changed to ${TARGETPLATFORM} and uses golang:1.25; RUN invoking ocb now prefixed with GOOS=${TARGETOS} GOARCH=${TARGETARCH}; ENTRYPOINT and CMD removed; smoke test retained.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Focus review on:
    • otelcollector/Dockerfile — verify build environment change, GOOS/GOARCH usage, and impact of removing ENTRYPOINT/CMD.
    • router/Dockerfile — confirm WORKDIR addition doesn't alter runtime expectations.
    • keycloak/Dockerfile — confirm EXPOSE and newline change are intentional.

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: switching Docker base images from BUILDPLATFORM to TARGETPLATFORM across multiple Dockerfiles to ensure proper multi-architecture builds.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8eacb6d and 4dd3805.

📒 Files selected for processing (8)
  • admission-server/Dockerfile (1 hunks)
  • cdn-server/Dockerfile (1 hunks)
  • controlplane/Dockerfile (1 hunks)
  • graphqlmetrics/Dockerfile (1 hunks)
  • keycloak/Dockerfile (2 hunks)
  • otelcollector/Dockerfile (1 hunks)
  • router/Dockerfile (2 hunks)
  • studio/Dockerfile (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • studio/Dockerfile
  • graphqlmetrics/Dockerfile
  • keycloak/Dockerfile
  • controlplane/Dockerfile
🧰 Additional context used
🪛 GitHub Check: build_push_image
cdn-server/Dockerfile

[warning] 24-24: Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior
RedundantTargetPlatform: Setting platform to predefined ${TARGETPLATFORM} in FROM is redundant as this is the default behavior
More info: https://docs.docker.com/go/dockerfile/rule/redundant-target-platform/

⏰ 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). (19)
  • GitHub Check: build-router
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: integration_test (./events)
  • GitHub Check: build_test
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: build_push_image
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: build_push_image
  • GitHub Check: image_scan
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: build_test
  • GitHub Check: build_push_image
🔇 Additional comments (5)
admission-server/Dockerfile (1)

23-23: LGTM!

The final stage correctly uses --platform=${TARGETPLATFORM} for runtime execution, while the builder stage remains on BUILDPLATFORM. This aligns with the PR objective to ensure proper multi-architecture targeting.

cdn-server/Dockerfile (1)

24-24: LGTM, with awareness of static analysis note.

The final stage correctly switches to --platform=${TARGETPLATFORM}. Note that the static analysis tool flags this as redundant (since TARGETPLATFORM is Docker's default for final stages), but explicit declaration is beneficial for clarity and consistency across the repository's Dockerfiles. No action needed unless consistency with other projects dictates otherwise.

Confirm that explicit --platform=${TARGETPLATFORM} is the desired consistency pattern for this project across all Dockerfiles.

router/Dockerfile (1)

34-34: LGTM!

Both final stages correctly switch to --platform=${TARGETPLATFORM} (lines 34 and 48), aligning with the PR objective. The addition of WORKDIR / on line 42 is a good compatibility adjustment for distroless images. The builder stage properly uses BUILDPLATFORM with cross-compilation flags, ensuring efficient multi-architecture builds.

Also applies to: 42-42, 48-48

otelcollector/Dockerfile (2)

19-19: LGTM on platform strategy fix.

Line 19 now correctly uses --platform=${TARGETPLATFORM}, addressing the past review comment and aligning with the PR objective. Line 15's explicit GOOS=${TARGETOS} GOARCH=${TARGETARCH} prefix ensures the ocb build targets the correct platform.

Also applies to: 15-15


27-28: The ENTRYPOINT and CMD instructions are present at lines 27-28, not removed. No breaking change exists in this code. Multiple deployments across Helm charts (helm/cosmo/charts/otelcollector/templates/deployment.yaml) and Docker Compose files (docker-compose.yml, docker-compose.full.yml, docker-compose.cosmo.yml) depend on these default instructions without overriding them, confirming they remain functional and in use.

Likely an incorrect or invalid review comment.


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

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-86681caa8bd17b129f4462641a1617a6072da0c2-nonroot

@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 36.73%. Comparing base (923cd4f) to head (4dd3805).

Additional details and impacted files
@@            Coverage Diff             @@
##            main    #2401       +/-   ##
==========================================
+ Coverage   1.49%   36.73%   +35.23%     
==========================================
  Files        292      833      +541     
  Lines      46926   113476    +66550     
  Branches     431     4625     +4194     
==========================================
+ Hits         703    41687    +40984     
- Misses     45940    70193    +24253     
- Partials     283     1596     +1313     

☔ View full report in Codecov by Sentry.
📢 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52a4c63 and 289f1a1.

📒 Files selected for processing (1)
  • otelcollector/Dockerfile (1 hunks)
⏰ 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). (1)
  • GitHub Check: build_test
🔇 Additional comments (1)
otelcollector/Dockerfile (1)

27-28: Address summary inconsistency regarding ENTRYPOINT and CMD.

The AI-generated summary claims that ENTRYPOINT ["/otelcol"] and CMD ["--config", "/etc/otel-config.yaml"] were removed, but they remain in the code at lines 27–28. Clarify whether these lines were intentionally kept (summary is outdated) or if the code change is incomplete.

@endigma endigma force-pushed the jesse/docker-target-platform-builds branch from 5959f2e to 733fb7b Compare December 9, 2025 14:26
@endigma endigma force-pushed the jesse/docker-target-platform-builds branch 2 times, most recently from 7dd87bc to 8eacb6d Compare December 10, 2025 14:48
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
cdn-server/Dockerfile (1)

1-1: Correct multi-arch build strategy, but reconsider explicit TARGETPLATFORM.

The change correctly uses BUILDPLATFORM for the builder stage (native compilation) and TARGETPLATFORM for the final stage (runtime platform). However, static analysis flags that explicitly setting TARGETPLATFORM is redundant—it's Docker's default behavior when no platform is specified.

Consider two approaches:

  1. Keep the explicit platform for clarity (documents intent).
  2. Remove --platform=${TARGETPLATFORM} to rely on the default, reducing redundancy.

The PR description indicates a systematic fix across multiple Dockerfiles. Verify the chosen approach is consistent across all modified files.

Also applies to: 24-24

controlplane/Dockerfile (1)

1-1: Same pattern: correct multi-arch strategy, redundant explicit platform flag.

Line 35 mirrors the same change in cdn-server/Dockerfile. The final stage correctly switches from BUILDPLATFORM to TARGETPLATFORM, but static analysis identifies the explicit --platform=${TARGETPLATFORM} as redundant since TARGETPLATFORM is the Docker default when building for a specific target platform.

If the pattern is intentional for documentation/clarity across the codebase, approve as-is. If it's an oversight, consider removing the explicit flag to streamline the Dockerfiles.

Also applies to: 35-35

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7dd87bc and 8eacb6d.

📒 Files selected for processing (8)
  • admission-server/Dockerfile (1 hunks)
  • cdn-server/Dockerfile (1 hunks)
  • controlplane/Dockerfile (1 hunks)
  • graphqlmetrics/Dockerfile (1 hunks)
  • keycloak/Dockerfile (2 hunks)
  • otelcollector/Dockerfile (1 hunks)
  • router/Dockerfile (2 hunks)
  • studio/Dockerfile (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • admission-server/Dockerfile
  • studio/Dockerfile
  • otelcollector/Dockerfile
  • graphqlmetrics/Dockerfile
  • router/Dockerfile
  • keycloak/Dockerfile
🧰 Additional context used
🪛 GitHub Check: build_push_image
cdn-server/Dockerfile

[warning] 24-24: Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior
RedundantTargetPlatform: Setting platform to predefined ${TARGETPLATFORM} in FROM is redundant as this is the default behavior
More info: https://docs.docker.com/go/dockerfile/rule/redundant-target-platform/

⏰ 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). (19)
  • GitHub Check: build-router
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: integration_test (./events)
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: image_scan
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: build_test
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: build_push_image
  • GitHub Check: build_push_image
  • GitHub Check: build_push_image
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)

@endigma endigma force-pushed the jesse/docker-target-platform-builds branch from 8eacb6d to 4dd3805 Compare December 15, 2025 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants