Conversation
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
|
| Branch | Total Count |
|---|---|
| main | 217 |
| This PR | 289 |
| Difference | +72 (33.18%) |
Keep up the great work migrating from Earthly to Earthbuild! 🚀
💡 Tips for finding more occurrences
Run locally to see detailed breakdown:
./.github/scripts/count-earthly.shNote that the goal is not to reach 0.
There is anticipated to be at least some occurences of earthly in the source code due to backwards compatibility with config files and language constructs.
Summary of ChangesHello @gilescope, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive set of new files to define and configure a specialized Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a custom buildkitd image for earthbuild with several supporting scripts and templates. While the overall architecture is sound, there are several critical issues that need to be addressed: a typo in the Go version for the debugger build, missing dependencies (bc) in the Dockerfile, a logic error in the cache size calculation, and a potential script failure due to set -e combined with grep and missing directory checks. Additionally, there are security concerns regarding unverified binary downloads in the auto-install script.
| RUN apk add --update --no-cache \ | ||
| cni-plugins@edge-community \ | ||
| gettext \ | ||
| git-lfs \ | ||
| iptables \ | ||
| jq \ | ||
| openssh-client \ | ||
| pigz \ | ||
| util-linux \ | ||
| xz |
There was a problem hiding this comment.
The entrypoint.sh script relies on the bc command for floating-point arithmetic and cache calculations (e.g., lines 176, 198, 199, 210). However, bc is not included in the apk add list, which will cause the entrypoint to fail during container startup.
RUN apk add --update --no-cache \
bc \
cni-plugins@edge-community \
gettext \
git-lfs \
iptables \
jq \
openssh-client \
pigz \
util-linux \
xz
| ln -sf "/sbin/$IP_TABLES" /sbin/iptables | ||
|
|
||
| # clear any leftovers (that aren't explicitly cached) in the dind dir | ||
| find /tmp/earthbuild/dind/ -maxdepth 1 -mindepth 1 | grep -v cache_ | xargs -r rm -rf |
There was a problem hiding this comment.
This line has two issues: 1) If /tmp/earthbuild/dind/ does not exist (e.g., on the first run), find will return an error and the script will exit due to set -e. 2) If no files match the grep -v filter, grep will return exit code 1, also causing the script to exit. Using find with a name filter and a directory check is more robust.
| find /tmp/earthbuild/dind/ -maxdepth 1 -mindepth 1 | grep -v cache_ | xargs -r rm -rf | |
| [ -d /tmp/earthbuild/dind/ ] && find /tmp/earthbuild/dind/ -maxdepth 1 -mindepth 1 ! -name "cache_*" -exec rm -rf {} + || true |
| fi | ||
|
|
||
| # Calculate the cache for source files to be 10% of the overall cache | ||
| SOURCE_FILE_KEEP_BYTES="$(echo "($CACHE_SIZE_MB * 1024 * 1024 * 0.5) / 1" | bc)" # Note /1 division truncates to int |
There was a problem hiding this comment.
There is a logic error in the cache calculation. The comment states that the source file cache should be 10% of the overall cache, but the code uses a multiplier of 0.5 (50%).
| SOURCE_FILE_KEEP_BYTES="$(echo "($CACHE_SIZE_MB * 1024 * 1024 * 0.5) / 1" | bc)" # Note /1 division truncates to int | |
| SOURCE_FILE_KEEP_BYTES="$(echo "($CACHE_SIZE_MB * 1024 * 1024 * 0.1) / 1" | bc)" # Note /1 division truncates to int |
| curl -L "https://github.com/linuxserver/docker-docker-compose/releases/download/1.27.4-ls27/docker-compose-armhf" -o /usr/local/bin/docker-compose | ||
| ;; | ||
| arm64|aarch64) | ||
| # renovate: datasource=github-releases packageName=linuxserver/docker-docker-compose | ||
| curl -L "https://github.com/linuxserver/docker-docker-compose/releases/download/1.27.4-ls27/docker-compose-arm64" -o /usr/local/bin/docker-compose | ||
| ;; | ||
| *) | ||
| # renovate: datasource=github-releases packageName=docker/compose | ||
| curl -L "https://github.com/docker/compose/releases/download/1.27.4/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose |
There was a problem hiding this comment.
Downloading binaries directly from a third-party repository without verifying their checksums (e.g., SHA256) poses a significant security risk. An attacker could compromise the source or perform a MITM attack to execute arbitrary code. Please add checksum verification for all downloaded binaries.
| ARG BASE_TAG=latest | ||
|
|
||
| # Build the earth_debugger from earthbuild2 source. | ||
| FROM golang:1.25-alpine AS debugger |
There was a problem hiding this comment.
The Go version 1.25 is not yet released (current stable versions are 1.23 and 1.24). This is likely a typo and should be updated to a valid version.
FROM golang:1.24-alpine AS debugger
|
We don't need to do this. buildkit is published from the earthbuild repo. |
2 participants
There's a chicken and egg. Doing this to force a arm64 buildkit image.