Migrate build system from Earthly to Nix#912
Conversation
📝 WalkthroughWalkthroughTransitions CI from Earthly to Nix/Cachix, adds a containerized Nix wrapper, introduces a flake.nix Nix Flakes configuration, and modernizes Go code using Go 1.22+ stdlib utilities while adding some deprecation comments and minor test/lint adjustments. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Would you like me to focus the review on Nix/CI changes, Go stdlib modernizations, or lint/config updates first? 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/reconciler/managed/reconciler_modern_test.go (2)
1873-1886:⚠️ Potential issue | 🟡 MinorAlign management policy values with the test-case names
In the
ManagementPolicyCreateCreateSuccessfulandManagementPolicyUpdateUpdateSuccessfulcases (Line 1873-1885 and Line 2017-2026), the managed object is set toManagementActionAll. If the intent is to exercise Create-only / Update-only behavior, should these useManagementActionCreateandManagementActionUpdaterespectively? That would make the scenarios match the why in their names. Thanks!Proposed adjustment
- mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) + mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionCreate}) - want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) + want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionCreate}) - mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) + mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionUpdate}) - want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) + want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionUpdate})Also applies to: 2017-2027
2174-2176:⚠️ Potential issue | 🟡 MinorConsider the purpose of
test.EquateErrors()before switching tocmpopts.EquateErrors()At lines 2175 and 2287, the test uses
test.EquateErrors(). While the coding guidelines specifycmpopts.EquateErrors(), we should clarify whether switching is appropriate here:test.EquateErrors()is a deliberate custom helper in this codebase that compares both error type and error string, whereascmpopts.EquateErrors()only checkserrors.Is()semantics without comparing messages. If testing the exact error string is important (as the custom helper suggests), the current approach is intentional. Could you confirm whether the error message comparison is needed in this test, or ifcmpopts.EquateErrors()would suffice?
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 71-75: The guidance line in AGENTS.md currently says "Use goCamel
case for JSON struct tags" which is a typo; update that line to read "Use
camelCase (lowerCamelCase) for JSON struct tags" so the recommendation is clear
and consistent; locate the Kubernetes Conventions section and replace the phrase
"goCamel case" with "camelCase (lowerCamelCase)".
In `@nix.sh`:
- Around line 28-55: Create a cleanup function (e.g., cleanup) that contains the
rsync/symlink-to-directory logic, the find ... -exec chown ... cleanup, and
removal of the marker; set a trap 'cleanup' EXIT immediately after creating
marker so cleanup always runs on success or failure of the subsequent nix "${@}"
call; ensure the trap does not swallow the original nix exit code (capture and
re-exit with it if you check/propagate exit status) and reference the existing
symbols: marker, the nix "${@}" invocation, the rsync block, and the find ...
-newer ... -exec chown ... sequence when moving logic into cleanup and
registering trap EXIT.
🧹 Nitpick comments (6)
.golangci.yml (1)
223-228: Consider scoping the SA1019 suppression bypathtopkg/reconciler/to limit its effect.
Thanks for documenting the legacy compatibility rationale. The current regex'SA1019: resource\.Legacy.+ is deprecated:'is appropriately specific. Sinceresource.LegacyManagedandresource.LegacyProviderConfigUsageare only used in thepkg/reconciler/subdirectory, addingpath: pkg/reconciler/to the exclusion rule would further isolate this suppression and reduce the risk of accidentally hiding unrelated deprecation warnings elsewhere. This is a minor improvement for future maintainability.pkg/reference/namespaced_reference.go (1)
287-296: Nice use of generics for the sorting helper!This is a clean, reusable implementation. I noticed that
reference.gohas a similar but non-genericsortMapByKeysfunction (specific toxpv1.Reference). Would it make sense to consolidate these into a single generic helper, or was the type-specific version inreference.gointentional for some reason?No action required if this is deliberate—just curious about the reasoning! 🙂
pkg/reference/reference.go (1)
414-428: Consistent implementation with the namespaced variant—nice!The valueMap approach here mirrors the pattern in
namespaced_reference.go, which is great for consistency. The same question about key collisions applies here: if two resources yield the same extracted value, only one reference will be retained in the map.If this is intentional (perhaps extracted values are expected to be unique), a brief comment noting this assumption would help future readers understand the design decision.
.github/renovate-entrypoint.sh (1)
1-12: Consider adding a clarifying comment to explain why both tools are installed.The renovate configuration is elegantly split—renovate-nix.json5 runs Nix post-upgrade tasks only on main (via
matchBaseBranches), while renovate-earthly.json5 runs Earthly tasks only on release branches. Both toolchains are correctly installed unconditionally in this container since it may handle any branch during a single run.A brief comment in the script explaining this design would help future maintainers understand why both installations are needed, since the current comments hint at branch-specific usage. Something like: "Both Nix and Earthly are installed since renovate config is branch-aware and determines which post-upgrade commands execute via matchBaseBranches filters."
.github/workflows/renovate.yml (1)
45-45: Consider narrowing the Renovate nix command allowlist.
Thanks for the update. Is there a reason to allow anynixcommand at this step (Line 45), or could we scope the regex to just the subcommands used inrenovate-entrypoint.shto reduce the command surface?nix.sh (1)
92-105: Consider pinning thenixos/niximage for reproducibility.Right now the Docker run uses the floating
nixos/nixtag. If you’re not intentionally tracking latest, a pinned tag or digest will prevent surprise changes across dev machines. Is there a version you want to lock to?
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
150-155:⚠️ Potential issue | 🔴 CriticalBug: Shell variable syntax won't expand in YAML context.
The
${GITHUB_REPOSITORY}uses shell variable interpolation syntax, but this isn't inside arun:block — it's a YAMLwith:parameter. GitHub Actions won't expand this, so the literal string${GITHUB_REPOSITORY}will be passed to the action, likely causing the breaking change detection to fail.Should this use GitHub's expression syntax instead?
Proposed fix
- name: Detect Breaking Changes in Protocol Buffers uses: bufbuild/buf-breaking-action@a074e988ee34efcd4927079e79c611f428354c01 # v1 # We want to run this for the main branch, and PRs against main. if: ${{ github.ref == 'refs/heads/main' || github.base_ref == 'main' }} with: - against: "https://github.com/${GITHUB_REPOSITORY}.git#branch=main" + against: "https://github.com/${{ github.repository }}.git#branch=main"
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
140-146: Consider SHA-pinning buf actions for consistency.I noticed
buf-setup-actionandbuf-lint-actionuse@v1tags while other actions in this workflow (checkout, codeql, trivy) are pinned to specific commit SHAs. For supply-chain security consistency, would it make sense to pin these as well?Not blocking, but it would align with the security posture demonstrated elsewhere in this file.
Example with SHA pinning
- name: Setup Buf - uses: bufbuild/buf-setup-action@v1 + uses: bufbuild/buf-setup-action@<SHA> # v1 with: github_token: ${{ secrets.GITHUB_TOKEN }} - name: Lint Protocol Buffers - uses: bufbuild/buf-lint-action@v1 + uses: bufbuild/buf-lint-action@<SHA> # v1You can find the current SHA for v1 tags via
gh api repos/bufbuild/buf-setup-action/git/ref/tags/v1 --jq .object.sha.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/reconciler/managed/reconciler_deprecated.go (1)
63-66:⚠️ Potential issue | 🟡 MinorSmall copy-paste duplicate in TODO comments
Thanks for consolidating these deprecated APIs! I noticed there's a duplicate line here:
// type TypedNopDisconnecter[managed resource.Managed] = TypedNopDisconnector[managed] // type TypedNopDisconnecter[managed resource.Managed] = TypedNopDisconnector[managed]Was this intentional, or did one of these lines slip in by accident? Just want to make sure the TODO comment reflects the intended list of future generic aliases.
Suggested fix
// TODO: these types of aliases are only allowed in Go 1.23 and above. // type TypedNopDisconnecter[managed resource.Managed] = TypedNopDisconnector[managed] -// type TypedNopDisconnecter[managed resource.Managed] = TypedNopDisconnector[managed] // type TypedExternalConnectDisconnecterFns[managed resource.Managed] = TypedExternalConnectDisconnectorFns[managed]pkg/reference/reference.go (1)
414-427:⚠️ Potential issue | 🟠 MajorPotential duplicate-value loss & nondeterministic pick when using a map.
Thanks for the deterministic ordering work here. One concern: keying by
req.Extract(to)will collapse duplicates. If two resources share the same extracted value (e.g., external names), the later entry wins and which one wins depends onListorder (not guaranteed), so results can be nondeterministic and valid duplicates are dropped. Is the extracted value guaranteed unique in this resolver? If not, consider sorting a slice of pairs instead of a map (and optionally tie‑break by name/namespace) to preserve duplicates deterministically.💡 Possible fix that preserves duplicates and determinism
- valueMap := make(map[string]xpv1.Reference) - - for _, to := range req.To.List.GetItems() { - if ControllersMustMatch(req.Selector) && !meta.HaveSameController(r.from, to) { - continue - } - - valueMap[req.Extract(to)] = xpv1.Reference{Name: to.GetName()} - } - - sortedKeys, sortedRefs := sortMapByKeys(valueMap) - - rsp := MultiResolutionResponse{ResolvedValues: sortedKeys, ResolvedReferences: sortedRefs} + type pair struct { + value string + ref xpv1.Reference + } + pairs := make([]pair, 0, len(req.To.List.GetItems())) + + for _, to := range req.To.List.GetItems() { + if ControllersMustMatch(req.Selector) && !meta.HaveSameController(r.from, to) { + continue + } + pairs = append(pairs, pair{ + value: req.Extract(to), + ref: xpv1.Reference{Name: to.GetName()}, + }) + } + + sort.Slice(pairs, func(i, j int) bool { + if pairs[i].value == pairs[j].value { + return pairs[i].ref.Name < pairs[j].ref.Name + } + return pairs[i].value < pairs[j].value + }) + + resolvedValues := make([]string, len(pairs)) + resolvedRefs := make([]xpv1.Reference, len(pairs)) + for i, p := range pairs { + resolvedValues[i] = p.value + resolvedRefs[i] = p.ref + } + + rsp := MultiResolutionResponse{ResolvedValues: resolvedValues, ResolvedReferences: resolvedRefs}
🤖 Fix all issues with AI agents
In `@pkg/reconciler/managed/reconciler_modern_test.go`:
- Around line 1787-1788: The test contains a duplicated chained call to
WithObservedGeneration on the ReconcileSuccess condition; update the assertion
so it only calls WithObservedGeneration(42) once (i.e., change
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42))
to use a single WithObservedGeneration(42)), keeping the SetConditions and
xpv1.ReconcileSuccess() calls unchanged.
- Around line 1975-1976: The test sets conditions using
want.SetConditions(xpv1.ReconcileSuccess().WithObservedGeneration(42).WithObservedGeneration(42))
but calls WithObservedGeneration twice; remove the duplicate call so the
condition chain only includes a single WithObservedGeneration(42) (keep
want.SetConditions, xpv1.ReconcileSuccess and WithObservedGeneration as the
referenced symbols).
🧹 Nitpick comments (2)
.coderabbit.yaml (1)
52-61: Consider including other Nix files in review scope.
Thanks for addingflake.nixto the filters. With the Nix migration, do you also wantnix/*.nix(e.g.,nix/apps.nix,nix/checks.nix) reviewed? If so, a broader include would avoid missing those changes.Suggested tweak
- "**/Dockerfile*" - "**/flake.nix" + - "nix/**/*.nix".github/workflows/ci.yml (1)
74-84: Consider adding an explicit CodeQL autobuild step for clarity.Go's CodeQL support includes built-in autobuild detection, so your workflow won't fail without an explicit step. However, for greater control and clarity, adding an autobuild step between
initandanalyzefollows best practices and makes the build process explicit. Your current setup with Nix should work, but would this explicit step align with how your project's Go compilation typically happens?Suggested improvement
- name: Initialize CodeQL uses: github/codeql-action/init@439137e1b50c27ba9e2f9befc93e43091b449c34 # v3 with: languages: go + - name: Autobuild + uses: github/codeql-action/autobuild@439137e1b50c27ba9e2f9befc93e43091b449c34 # v3 + - name: Perform CodeQL Analysis uses: github/codeql-action/analyze@439137e1b50c27ba9e2f9befc93e43091b449c34 # v3Alternatively, you can use
build-mode: autobuildin the init step if you prefer a single configuration point.
f37e8eb to
5bc31ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/renovate-entrypoint.sh:
- Around line 5-14: The script unconditionally downloads Earthly from "latest"
without checksum and always installs both Earthly and Nix despite comments
implying branch-specific behavior; update the Earthly section to pin a specific
release (replace "latest" with a concrete version string used in the repo) and
verify its SHA256 checksum before making it executable, and either add
branch-aware conditional logic (check an environment variable like GITHUB_REF or
a passed-in BRANCH value) so Earthly is only installed for release branches and
Nix only for main, or change the comments to state both are intentionally
installed; references: the Earthly download path (/usr/local/bin/earthly) and
the apt-get install -y nix-bin call.
In @.github/workflows/ci.yml:
- Around line 57-84: The workflow is missing a CodeQL build step so Go
extraction will be empty; insert a step using github/codeql-action/autobuild
between the existing "Initialize CodeQL" (uses: github/codeql-action/init) and
"Perform CodeQL Analysis" (uses: github/codeql-action/analyze) steps so CodeQL
performs a Go autobuild (detects go.mod and extracts the DB) before analysis;
ensure the new step runs after init and before analyze and does not conflict
with Nix by relying on the autobuild action.
🧹 Nitpick comments (3)
pkg/meta/meta.go (1)
180-189: Optional: Consider usingslices.DeleteFuncfor consistency?Just a thought for a potential follow-up —
RemoveFinalizercould also benefit from the standard library treatment usingslices.DeleteFunc. This would make the file consistent and also correctly handle the (admittedly unlikely) edge case of duplicate finalizers:o.SetFinalizers(slices.DeleteFunc(f, func(e string) bool { return e == finalizer }))Totally fine to defer this if you'd prefer to keep the PR focused. What do you think?
.coderabbit.yaml (1)
52-61: Consider including all Nix files in the review scope.Right now only
flake.nixis included, sonix/*.nixwon’t be reviewed. Was the narrow scope intentional? If not, broadening the pattern keeps the Nix tooling visible. Thanks!♻️ Suggested tweak
- - "**/flake.nix" + - "**/*.nix".golangci.yml (1)
224-229: Would scoping the legacy deprecation suppression be feasible?If the deprecations are limited to specific packages, scoping by
path:could avoid masking unrelated warnings. Is that something you’d want here? Thanks!
Earthly's maintainers announced they would no longer actively develop the tool. This commit migrates crossplane-runtime to use Nix, matching the approach taken in crossplane/crossplane#6999. The migration adds a Nix flake with: - Apps for local development (test, lint, generate, tidy) - Checks for CI (test, go-lint, generate, nix-lint) - A development shell with all required tools - A Docker wrapper (nix.sh) that requires no local Nix installation This commit also updates apis/apis.go to invoke controller-gen and buf directly from $PATH rather than via `go run`. This matches crossplane's pattern and allows code generation to work in Nix's sandboxed builds. The CI workflow now uses Nix checks, and CodeQL uses GitHub's native action rather than a custom Earthly target. The Renovate configuration is split into modular files (base, nix, earthly) to support both Nix on main and Earthly on release branches. The Earthfile is retained for release branches that still use it. Note: The go-lint check will fail until pre-existing lint issues (22 gocritic and godoclint violations) are addressed in a follow-up commit. Signed-off-by: Nic Cope <nicc@rk0n.org>
This just adds a bunch of whitespace, mostly before returns. We have it disabled in c/c - it's not something we value. Signed-off-by: Nic Cope <nicc@rk0n.org>
This auto-fixes several linter issues. Signed-off-by: Nic Cope <nicc@rk0n.org>
golangci-lint 2.6 flags deprecation notices that aren't in their own paragraph, and duplicate package documentation across files. This commit fixes deprecated comment formatting by adding blank lines before Deprecated: notices, and removes duplicate package docs from event_test.go and namespaced_reference.go. It also adds lint exclusions for: - goconst in test files (repeated test reason strings are intentional) - staticcheck SA1019 for resource.Legacy* types (the runtime must use these deprecated types to support legacy resources) Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Code generation directives were in apis/apis.go, which mixed tooling concerns with API package documentation. This made it less obvious where generation was configured. This commit moves the //go:generate directives to a dedicated generate.go file at the repo root, matching crossplane/crossplane's structure. The buf configuration files also move to root, since buf now runs from there. The generated protobuf files have updated source paths (proto/... to apis/proto/...) but are otherwise unchanged. Signed-off-by: Nic Cope <nicc@rk0n.org>
The buf configuration moved from apis/ to root as part of the generate.go refactor. The CI buf actions were still using input: apis, which caused them to fail since there's no buf.yaml in apis/ anymore. Signed-off-by: Nic Cope <nicc@rk0n.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Line 155: The `with:` value for the buf-breaking-action currently uses
shell-style `${GITHUB_REPOSITORY}` which won't be expanded by Actions; change
the string passed to the action's `against:` input to use GitHub Actions
expressions (e.g. replace the literal `${GITHUB_REPOSITORY}` in the `against:
"https://github.com/${GITHUB_REPOSITORY}.git#branch=main"` setting with the
Actions expression for the repository, such as `${{ github.repository }}`, so
the buf-breaking-action receives the actual repo URL and branch).
🧹 Nitpick comments (1)
.github/workflows/renovate.yml (1)
45-45: Could we narrow the allowed nix commands?Thanks for enabling Nix for Renovate. To keep the allowlist tight, would you be open to restricting the regex to only the subcommands Renovate actually runs (for example,
nix run .#...andnix flake check)? That helps reduce the command surface area if configs drift.🔒 Example of a narrower allowlist
- RENOVATE_ALLOWED_POST_UPGRADE_COMMANDS: '["^nix .+", "^earthly .+"]' + RENOVATE_ALLOWED_POST_UPGRADE_COMMANDS: '["^nix (run \\.#(test|lint|generate|tidy)|flake check)$", "^earthly .+"]'
adamwg
left a comment
There was a problem hiding this comment.
One note on the generate check; otherwise, this lgtm (and I think I'm starting to understand the nix language).
The generate check only compared apis/ against committed source, but go generate also produces files in pkg/ (zz_generated.deepcopy.go). Diff the entire tree to catch any generated file drift. The vendor directory is excluded since gomod2nix creates it during sandboxed builds. Signed-off-by: Nic Cope <nicc@rk0n.org>
Follow-up to crossplane#912. Signed-off-by: Nic Cope <nicc@rk0n.org>
Follow-up to crossplane#912. Signed-off-by: Nic Cope <nicc@rk0n.org>
Description of your changes
Matches crossplane/crossplane#6999 for crossplane-runtime.
Earthly's maintainers announced last year that they would no longer actively develop it. This PR replaces Earthly with a Nix flake.
The Renovate config is split into modular files so main uses Nix while release branches continue using Earthly. Code generation moves from
apis/apis.goto a dedicatedgenerate.goat the repo root, with buf config files also at root.I have:
./nix.sh flake checkto ensure this PR is ready for review.Added or updated unit tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.