feat: replace Earthly #2154
feat: replace Earthly #2154
Conversation
julienmancuso
requested review from
biswapanda,
hhzhang16,
hutm,
ishandhanani,
mohammedabdulwahhab and
nnshah1
as code owners
WalkthroughA new multi-stage Dockerfile and Makefile are introduced, establishing a comprehensive build system for a project with Python, Rust, and Go components. Existing Earthfiles are deprecated in favor of Make-based workflows. Documentation and deployment guides are updated to reflect the migration from Earthly to Make for building and pushing Docker images. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Makefile
participant Docker
participant Rust
participant Python
participant Go
participant DockerHub
Developer->>Makefile: make build
Makefile->>Rust: Build Rust components
Makefile->>Python: Build Python wheels
Makefile->>Go: Build operator binary
Makefile->>Docker: Build Docker images
Makefile->>DockerHub: (optional) Push images
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
deploy/cloud/operator/Dockerfile (1)
5-7: Shrink builder image by avoiding recommended packagesUsing plain
apt-get install -y gitpulls many recommended packages that are not required for a build container and increases image size. Add the--no-install-recommendsflag for a slimmer layer.-RUN apt-get update && apt-get install -y git && \ +RUN apt-get update && apt-get install -y --no-install-recommends git && \ apt-get clean && rm -rf /var/lib/apt/lists/*Dockerfile (1)
55-60: Hard-coded Rustup binary & checksum will age quicklyYou embed a SHA256 for
rustup-init1.28.1. When a new rustup release is required the digest will break the build. To keep the build stable yet maintainable:
- Accept the hash via
--build-arg RUSTUP_SHA256, defaulting to the current value.- Document how to update it.
Optional, but avoids future breakage each time rustup updates.
deploy/cloud/helm/README.md (1)
46-47: Use explicit Makefile variables for reproducibilityInstead of relying on exported env vars from the shell session, you can invoke make with variable overrides:
make docker-build-operator docker-push-operator \ DOCKER_SERVER="$DOCKER_SERVER" IMAGE_TAG="$IMAGE_TAG"This makes the one-liner copy/pasteable and avoids polluting the user’s environment.
docs/guides/dynamo_deploy/quickstart.md (1)
100-107: Avoid duplicated environment-variable setup & make invocation clearer
DOCKER_SERVER/IMAGE_TAGare already exported in the earlier “Set Environment Variables” block (lines 73-77). Re-exporting them here can confuse readers and risks divergence if they change one place but not the other.
Consider either deleting lines 103-105 or replacing them with a reminder such as “(reuse the variables set above)” and add a short note thatmake docker-build-operator docker-push-operatormust be executed from the repository root where the Makefile lives.Earthfile (1)
16-26: Strip trailing whitespace in the new deprecation headerCI flagged trailing spaces on this block (see pipeline error). A quick fix:
-# - Use `container/build.sh` for framework containers +# - Use `container/build.sh` for framework containersRun
pre-commit run --all-fileslocally to catch these automatically.Makefile (3)
28-39:install-depsuses separate shells – PATH update may be lostEach recipe line runs in its own shell, so the
source $$HOME/.cargo/envinside theifblock does not persist to later lines (or other targets). If the user didn’t havecargoon their PATH beforehand, subsequent targets will fail.Combine the commands into one shell or export PATH explicitly, e.g.:
- @if ! command -v rustup >/dev/null 2>&1; then \ - curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y; \ - source $$HOME/.cargo/env; \ - fi + @if ! command -v rustup >/dev/null 2>&1; then \ + curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \ + echo 'export PATH=$$HOME/.cargo/bin:$$PATH' >> ~/.profile; \ + fi
146-151: Add conventionalalltarget to silence checkmake warningStatic-analysis complains that a phony
alltarget is missing. Adding a thin wrapper keeps linters quiet and gives users an obvious entry point:+.PHONY: all +all: build ## Build everything (alias for 'make build')
170-177: Trailing whitespace at lines 172/177 breaks pre-commit- earthly +all-docker $(EARTHLY_ARGS) + earthly +all-docker $(EARTHLY_ARGS)This will clear the “trailing-whitespace” hook failure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Dockerfile(1 hunks)Earthfile(1 hunks)Makefile(1 hunks)deploy/cloud/helm/README.md(1 hunks)deploy/cloud/operator/Dockerfile(1 hunks)deploy/cloud/operator/Earthfile(1 hunks)docs/guides/dynamo_deploy/dynamo_cloud.md(1 hunks)docs/guides/dynamo_deploy/quickstart.md(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1308-1312
Timestamp: 2025-06-11T21:29:28.650Z
Learning: User julienmancuso expects replies in English; avoid switching languages unless explicitly requested.
docs/guides/dynamo_deploy/dynamo_cloud.md (1)
Learnt from: julienmancuso
PR: #1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1302-1306
Timestamp: 2025-06-11T21:18:00.425Z
Learning: In the Dynamo operator, the project’s preferred security posture is to set a Pod-level PodSecurityContext with runAsUser, runAsGroup, and fsGroup all set to 1000, and then selectively override the user at the individual container level (e.g., RunAsUser: 0 for Kaniko) when root is required.
deploy/cloud/operator/Earthfile (1)
Learnt from: julienmancuso
PR: #2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:92-98
Timestamp: 2025-07-18T16:04:31.771Z
Learning: CRD schemas in files like deploy/cloud/helm/crds/templates/*.yaml are auto-generated from Kubernetes library upgrades and should not be manually modified as changes would be overwritten during regeneration.
docs/guides/dynamo_deploy/quickstart.md (2)
Learnt from: julienmancuso
PR: #1337
File: deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml:0-0
Timestamp: 2025-06-03T15:26:55.732Z
Learning: The image-builder ServiceAccount in deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml does not need imagePullSecrets, unlike the component ServiceAccount.
Learnt from: julienmancuso
PR: #1365
File: deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go:171-178
Timestamp: 2025-06-04T13:09:53.416Z
Learning: The DYN_DEPLOYMENT_CONFIG environment variable (commonconsts.DynamoDeploymentConfigEnvVar) in the Dynamo operator will never be set via ValueFrom (secrets/config maps), only via direct Value assignment. The GetDynamoDeploymentConfig method correctly only checks env.Value for this specific environment variable.
deploy/cloud/helm/README.md (2)
Learnt from: julienmancuso
PR: #1337
File: deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml:0-0
Timestamp: 2025-06-03T15:26:55.732Z
Learning: The image-builder ServiceAccount in deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml does not need imagePullSecrets, unlike the component ServiceAccount.
Learnt from: julienmancuso
PR: #2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:92-98
Timestamp: 2025-07-18T16:04:31.771Z
Learning: CRD schemas in files like deploy/cloud/helm/crds/templates/*.yaml are auto-generated from Kubernetes library upgrades and should not be manually modified as changes would be overwritten during regeneration.
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/2154/merge) by julienmancuso.
deploy/cloud/operator/Earthfile
[error] 18-18: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. Run 'pre-commit run --all-files' locally to reproduce.
Earthfile
[error] 18-18: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. Run 'pre-commit run --all-files' locally to reproduce.
deploy/cloud/operator/Dockerfile
[error] 23-23: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. Run 'pre-commit run --all-files' locally to reproduce.
[error] 89-89: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. Run 'pre-commit run --all-files' locally to reproduce.
Dockerfile
[error] 23-23: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. Run 'pre-commit run --all-files' locally to reproduce.
[error] 89-89: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. Run 'pre-commit run --all-files' locally to reproduce.
Makefile
[error] 174-174: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. Run 'pre-commit run --all-files' locally to reproduce.
🪛 markdownlint-cli2 (0.17.2)
deploy/cloud/helm/README.md
45-45: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
45-45: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
🪛 checkmake (0.2.2)
Makefile
[warning] 174-174: Missing required phony target "all"
(minphony)
⏰ 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). (2)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
docs/guides/dynamo_deploy/dynamo_cloud.md (1)
35-35: 👍 Good replacement of Earthly with MakeThe prerequisite list now correctly reflects the new build workflow. No issues found.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
LGTM overall. Few non-blocking nit-picking comments/questions.
biswapanda
left a comment
biswapanda
left a comment
There was a problem hiding this comment.
Approved to unblock CI urgently
- let's move the top level Dockerfile into
container/..
|
Also let's ensure the operator spins up in CI with new docker image |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Signed-off-by: Julien Mancuso <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]>
julienmancuso
dismissed
nv-tusharma’s stale review
MR has been updated since your initial review.
Signed-off-by: Julien Mancuso <[email protected]> Signed-off-by: Jason Zhou <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]> Signed-off-by: Jason Zhou <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]> Signed-off-by: Kyle H <[email protected]>
Overview:
replace Earthly
Summary by CodeRabbit
New Features
Documentation
Chores