-
-
Notifications
You must be signed in to change notification settings - Fork 60
fix(docker): add timezones database in base and scratch images #1950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Sylvain Rabot <[email protected]>
WalkthroughThe Dockerfile modifies the base stage to install tzdata and set DEBIAN_FRONTEND=noninteractive, and updates the final scratch stage to copy zoneinfo data. The certs stage remains functionally the same, with formatting-only adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Base as Base Stage
participant Certs as Certs Stage
participant Final as Final (scratch) Stage
Note over Base: Build base image
Base->>Base: apt-get install ca-certificates, curl, tzdata\n(DEBIAN_FRONTEND=noninteractive)
Note over Certs: Extract certificates
Certs->>Certs: Prepare /etc/ssl/certs
Note over Final: Assemble scratch image
Final->>Base: COPY /usr/share/zoneinfo
Final->>Certs: COPY /etc/ssl/certs
Note right of Final: Result: scratch image with CA certs + timezone data
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 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.
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 (3)
base.Dockerfile (3)
2-2: Use apt-get and drop unnecessary deps to slim the layerSwitch to apt-get and --no-install-recommends; curl seems unused for producing zoneinfo. Smaller, more reproducible.
-RUN apt update && DEBIAN_FRONTEND=noninteractive apt install -y ca-certificates curl tzdata && rm -rf /var/lib/apt/lists/* +RUN apt-get update \ + && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends ca-certificates tzdata \ + && rm -rf /var/lib/apt/lists/*If curl is actually needed later in the build, please confirm.
4-6: Pin Alpine and use --no-cache for reproducibility and smaller layersAvoid mutable latest and apk cache.
-FROM alpine:latest AS certs +FROM alpine:3.20.3 AS certs -RUN apk --update add ca-certificates +RUN apk add --no-cache ca-certificates
7-9: LGTM; zoneinfo in scratch is correct. Optional consolidationCopying /usr/share/zoneinfo fixes Go/POSIX tz lookups in scratch. Consider consolidating certs+tzdata into a single Alpine stage to drop the Ubuntu stage altogether.
Example:
FROM alpine:3.20.3 AS runner-deps RUN apk add --no-cache ca-certificates tzdata FROM scratch COPY --from=runner-deps /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt COPY --from=runner-deps /usr/share/zoneinfo /usr/share/zoneinfo
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
base.Dockerfile(1 hunks)
No description provided.