feat: Simplify nix-lib to focus only on building#8
Open
ausias-armesto wants to merge 12 commits intomainfrom
Open
feat: Simplify nix-lib to focus only on building#8ausias-armesto wants to merge 12 commits intomainfrom
ausias-armesto wants to merge 12 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request removes multi-architecture Docker support and upload functionality from the codebase, consolidating the public API to focus on Docker image building. It eliminates multi-arch manifests, upload scripts, and documentation, while adding a configurable Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
516-529:⚠️ Potential issue | 🟡 MinorCI snippet still shows registry upload +
IMAGE_TARGET.The new app only builds/loads. Update the example to use
IMAGE_NAMEand remove the upload wording (push can happen separately in CI).Suggested doc fix
-# Build everything -nix build .#docker-amd64 +# Build everything +nix build .#docker-amd64 nix build .#trivy-scan nix build .#sbom @@ -# Upload image to registry -GOOGLE_ACCESS_TOKEN="$(gcloud auth print-access-token)" \ -IMAGE_TARGET="gcr.io/my-project/my-app:v1.0.0" \ -nix run .#upload-amd64 +# Build and tag the image locally (push in CI if needed) +IMAGE_NAME="gcr.io/my-project/my-app:v1.0.0" \ +nix run .#build-amd64 +# docker push gcr.io/my-project/my-app:v1.0.0
🤖 Fix all issues with AI agents
In `@examples/rust-app/README.md`:
- Around line 72-73: The README currently says "Upload to registry" and shows
using IMAGE_TARGET; update the step to clarify it's building/tagging (not
uploading) and use IMAGE_NAME as the environment variable the build expects:
change the header text to something like "Build and tag image (optional registry
tag)" and update the example command to use IMAGE_NAME with the nix target
build-docker (e.g., IMAGE_NAME=... nix run .#build-docker), ensuring any mention
of uploading/pushing is removed or prefaced with an explicit separate push step
if needed.
In `@lib/scripts/docker-build.sh`:
- Around line 59-75: The script only extracts a tag from LOAD_OUTPUT by matching
"Loaded image:" so it misses the "Loaded image ID:" form; update the
LOADED_IMAGE extraction to handle both "Loaded image:" and "Loaded image ID:"
(split on ": " and take the right-hand field) so LOADED_IMAGE is set whether
docker load prints a repo:tag or an image ID, and then ensure the IMAGE_NAME
tagging block verifies LOADED_IMAGE is non-empty before invoking docker tag and
fails with a clear error if IMAGE_NAME is provided but LOADED_IMAGE could not be
resolved; reference symbols: LOADED_IMAGE, LOAD_OUTPUT, IMAGE_NAME, and the
docker tag invocation.
In `@README.md`:
- Around line 297-306: Update the README example for mkDockerBuildApp to use the
environment variable name expected by the script: replace the usage comment that
references IMAGE_TARGET with IMAGE_NAME and ensure the example shows setting
IMAGE_NAME when running apps.upload-image (referencing mkDockerBuildApp,
apps.upload-image, IMAGE_NAME).
🧹 Nitpick comments (2)
README.md (2)
403-404: Consider renaming the example app to avoid “upload” semantics.The app only builds/loads; “build” naming is clearer.
Optional rename
-apps.upload = lib.mkDockerBuildApp myImage; +apps.build = lib.mkDockerBuildApp myImage;
494-496: Consider renamingupload-amd64to match the build-only flow.This avoids implying registry upload in examples.
Optional rename
-# Build single architecture image -upload-amd64 = lib.mkDockerBuildApp imageAmd64; +# Build single architecture image +build-amd64 = lib.mkDockerBuildApp imageAmd64;
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/docker.nix (1)
45-45: Useinheritfor self-referential attribute assignment.
pathsToLink = pathsToLink;is equivalent toinherit pathsToLink;and the latter is idiomatic Nix.♻️ Proposed fix
copyToRoot = pkgs.buildEnv { name = "image-root"; paths = actualBasePackages ++ extraContents; - pathsToLink = pathsToLink; + inherit pathsToLink; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/docker.nix` at line 45, The attribute assignment uses an explicit self-reference "pathsToLink = pathsToLink;" which is non-idiomatic Nix; replace it with the idiomatic self-inherit form by using "inherit pathsToLink;" in the attribute set where "pathsToLink" is defined so the attribute is bound to the same name more succinctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/docker.nix`:
- Line 45: The attribute assignment uses an explicit self-reference "pathsToLink
= pathsToLink;" which is non-idiomatic Nix; replace it with the idiomatic
self-inherit form by using "inherit pathsToLink;" in the attribute set where
"pathsToLink" is defined so the attribute is bound to the same name more
succinctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
18-18:⚠️ Potential issue | 🟡 MinorStale "Docker upload scripts" in the Features list.
After the rename from upload to build, this bullet still references "upload scripts."
📝 Proposed fix
- - **Utility apps**: Docker upload scripts, security audits, and more + - **Utility apps**: Docker build scripts, security audits, and more🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 18, The README features list contains a stale reference "Docker upload scripts" under the "**Utility apps**" bullet; update that bullet to reflect the rename from "upload" to "build" (e.g., change the text to "Docker build scripts, security audits, and more") so the Features list matches the current terminology. Locate the "**Utility apps**" bullet in README.md and replace "upload scripts" with "build scripts" (or equivalent phrasing) consistently.
♻️ Duplicate comments (1)
README.md (1)
297-303:⚠️ Potential issue | 🟡 Minor
mkDockerBuildAppsection lacks a usage example showingIMAGE_NAME.The previous env-var usage comment was removed rather than corrected, so the docs now give no indication of how to actually run the app.
📝 Proposed addition
apps.docker-image = lib.mkDockerBuildApp myDockerImage; + +# Usage: +# IMAGE_NAME=gcr.io/project/image:tag nix run .#docker-image🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 297 - 303, Update the mkDockerBuildApp docs to include a concrete usage example demonstrating the IMAGE_NAME environment variable; in the README's mkDockerBuildApp section mention how to run the generated app (e.g., setting IMAGE_NAME before invoking the app) and show a short example command so readers know how to supply the image name when building; reference mkDockerBuildApp and the IMAGE_NAME env var in the text so it's clear which variable to set.
🧹 Nitpick comments (2)
lib/sources.nix (1)
85-115:testDataPatternsis declared but never applied —mkTestSrcis functionally identical tomkSrc.The
testDataPatternsparameter (line 91) is accepted but never referenced in thefilesetconstruction (lines 100–111). Any caller that passes patterns expecting test fixtures to be included will silently get the same fileset asmkSrc, making their test builds miss the intended test data.Based on learnings,
mkTestSrcis meant to handle test data inclusions. The parameter should either be applied or the function should delegate tomkSrcand extend it:♻️ Suggested fix: apply `testDataPatterns` and delegate to `mkSrc`
mkTestSrc = { root, fs, extraFiles ? [ ], extraExtensions ? [ ], testDataPatterns ? [ ], }: - let - baseExtensions = [ - "rs" - "toml" - ]; - allExtensions = baseExtensions ++ extraExtensions; - - fileset = fs.unions ( - [ - # Cargo configuration - (fs.maybeMissing (root + "/.cargo/config.toml")) - (root + "/Cargo.lock") - (root + "/README.md") - - # Source files - ] - ++ (map (ext: fs.fileFilter (file: file.hasExt ext) root) allExtensions) - ++ extraFiles - ); - in - fs.toSource { - inherit root fileset; - }; + let + baseSrc = mkSrc { inherit root fs extraFiles extraExtensions; }; + testFilesets = map (pat: fs.fileFilter (file: file.name == pat) root) testDataPatterns; + in + if testDataPatterns == [ ] then + baseSrc + else + fs.toSource { + inherit root; + fileset = fs.unions ([ baseSrc ] ++ testFilesets); + };Note: adjust the pattern-matching strategy (e.g.
fileFilterby extension or path glob) to match the intended semantics oftestDataPatterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sources.nix` around lines 85 - 115, mkTestSrc currently ignores the testDataPatterns parameter so its fileset is identical to mkSrc; update mkTestSrc to include test data by either delegating to mkSrc and extending its fileset or by adding filters built from testDataPatterns into the fileset construction: for example, map each pattern in testDataPatterns to an fs.fileFilter that matches file.path (or the intended glob/substring semantics) and append those results to the fileset (alongside the existing extension-based filters and extraFiles), ensuring the new filters reference mkTestSrc's testDataPatterns and preserve existing baseExtensions/allExtensions behavior.README.md (1)
490-498: Consider addingdocker-image-arm64app to match the definedimageArm64package.The example builds and exports both
imageAmd64andimageArm64as packages, but only creates an app for the amd64 variant. If the intent is to show single-arch at a time, a clarifying comment would help; otherwise adding the arm64 variant makes the example self-consistent.📝 Proposed addition (if multi-arch apps are desired)
apps = { # Build single architecture image docker-image-amd64 = lib.mkDockerBuildApp imageAmd64; + docker-image-arm64 = lib.mkDockerBuildApp imageArm64; # Security audit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 490 - 498, The apps map only defines docker-image-amd64 while the README also shows an imageArm64 package; either add a matching app entry (e.g., add docker-image-arm64 = lib.mkDockerBuildApp imageArm64; to the apps block) so both arch packages are represented, or if you intend to show only single-arch at a time, add a clarifying comment near docker-image-amd64 explaining that the example intentionally demonstrates a single-arch app build; reference the apps map and the imageArm64/imageAmd64 identifiers when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 511-520: The README has an unclosed fenced code block opened with
```bash before the build commands, which causes everything from the "## Platform
Support" heading onward to render as code; close the fence by adding the missing
closing ``` immediately after the build command list (i.e. close the ```bash
block that surrounds the build lines) so the "## Platform Support" heading and
subsequent sections render normally.
---
Outside diff comments:
In `@README.md`:
- Line 18: The README features list contains a stale reference "Docker upload
scripts" under the "**Utility apps**" bullet; update that bullet to reflect the
rename from "upload" to "build" (e.g., change the text to "Docker build scripts,
security audits, and more") so the Features list matches the current
terminology. Locate the "**Utility apps**" bullet in README.md and replace
"upload scripts" with "build scripts" (or equivalent phrasing) consistently.
---
Duplicate comments:
In `@README.md`:
- Around line 297-303: Update the mkDockerBuildApp docs to include a concrete
usage example demonstrating the IMAGE_NAME environment variable; in the README's
mkDockerBuildApp section mention how to run the generated app (e.g., setting
IMAGE_NAME before invoking the app) and show a short example command so readers
know how to supply the image name when building; reference mkDockerBuildApp and
the IMAGE_NAME env var in the text so it's clear which variable to set.
---
Nitpick comments:
In `@lib/sources.nix`:
- Around line 85-115: mkTestSrc currently ignores the testDataPatterns parameter
so its fileset is identical to mkSrc; update mkTestSrc to include test data by
either delegating to mkSrc and extending its fileset or by adding filters built
from testDataPatterns into the fileset construction: for example, map each
pattern in testDataPatterns to an fs.fileFilter that matches file.path (or the
intended glob/substring semantics) and append those results to the fileset
(alongside the existing extension-based filters and extraFiles), ensuring the
new filters reference mkTestSrc's testDataPatterns and preserve existing
baseExtensions/allExtensions behavior.
In `@README.md`:
- Around line 490-498: The apps map only defines docker-image-amd64 while the
README also shows an imageArm64 package; either add a matching app entry (e.g.,
add docker-image-arm64 = lib.mkDockerBuildApp imageArm64; to the apps block) so
both arch packages are represented, or if you intend to show only single-arch at
a time, add a clarifying comment near docker-image-amd64 explaining that the
example intentionally demonstrates a single-arch app build; reference the apps
map and the imageArm64/imageAmd64 identifiers when making the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/rust-app/flake.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
README.mdexamples/rust-app/README.mdlib/sources.nix
💤 Files with no reviewable changes (1)
- examples/rust-app/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
15-15:⚠️ Potential issue | 🟡 MinorStale "upload" reference in the Features list.
The description still mentions "Docker upload scripts" but upload functionality has been removed in this PR.
📝 Proposed fix
-- **Utility apps**: Docker upload scripts, security audits, and more +- **Utility apps**: Docker build scripts, security audits, and more🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 15, The Features list in README.md still mentions "Docker upload scripts" which is stale; update the bullet that currently reads "**Utility apps**: Docker upload scripts, security audits, and more" to remove the "Docker upload scripts" phrase (e.g., replace with "Docker scripts" or just "Docker tools" or remove that part entirely) so the README reflects that upload functionality was removed; update the single line containing "Docker upload scripts" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 457-460: The README's CI example uses the wrong Nix target:
`docker-image-amd64` is defined under outputs.apps, so `nix build
.#docker-image-amd64` will fail; update the example to use the correct
command—either `nix run .#docker-image-amd64` to invoke the app in outputs.apps
(or `nix run .#default` for outputs.apps.default), or change to the package name
`docker-amd64` and use `nix build .#docker-amd64` to build the Docker image
package—ensure the README references the exact symbol (`docker-image-amd64` vs
`docker-amd64`) that exists in the Nix outputs.
---
Outside diff comments:
In `@README.md`:
- Line 15: The Features list in README.md still mentions "Docker upload scripts"
which is stale; update the bullet that currently reads "**Utility apps**: Docker
upload scripts, security audits, and more" to remove the "Docker upload scripts"
phrase (e.g., replace with "Docker scripts" or just "Docker tools" or remove
that part entirely) so the README reflects that upload functionality was
removed; update the single line containing "Docker upload scripts" accordingly.
tolbrino
approved these changes
Feb 24, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 446-448: The README's CI/CD example references an apps entry
`docker-image-amd64` that is not defined in the "complete example" (which only
defines `apps.audit`) while the Docker build output is actually provided under
`packages` as `docker-amd64`; add an apps binding named `docker-image-amd64`
that points to the existing `packages.docker-amd64` (or rename the package to
match) so `nix run .#docker-image-amd64` works; update the example block to
include the new apps entry alongside `audit` to keep the snippet self-contained
(references: apps, audit, docker-image-amd64, packages.docker-amd64,
docker-amd64).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
AGENTS.mdREADME.mdexamples/rust-app/flake.nixlib/apps.nixlib/default.nix
💤 Files with no reviewable changes (3)
- lib/apps.nix
- AGENTS.md
- examples/rust-app/flake.nix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplifies the nix-lib to focus mainly on building and letting the CI do the work for publishing into the registry, signing and scan vulnerability.
Fixes #4
Summary by CodeRabbit
Release Notes
Removed Features
New Features
Improvements
/bin)