Skip to content

Conversation

@jordanstephens
Copy link
Member

@jordanstephens jordanstephens commented Nov 27, 2025

this make task is run in pre-push hooks, so this should reduce CI churn because of out-of-sync nix vendor hashes

Description

Linked Issues

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

  • New Features

    • Added Nix-based CLI execution for improved environment consistency.
    • Introduced automated vendor dependency management with built-in recovery mechanisms.
  • Chores

    • Enhanced build and test workflows with improved Nix integration and dependency validation.

✏️ Tip: You can customize this high-level summary in your review settings.

this make task is run in pre-push hooks, so this should reduce CI churn
because of out-of-sync nix vendor hashes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces automatic updating of the nix vendor hash to reduce CI failures. When the test-nix target (run in pre-push hooks) detects a nix build failure, it automatically updates the vendorHash in pkgs/defang/cli.nix and stages the change.

Key changes:

  • Modified test-nix to automatically update vendor hash on failure
  • Added update-nix-vendor-hash target that manipulates vendorHash using sed
  • Extracted nix-run as a separate reusable target

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Co-authored-by: Lio李歐 <[email protected]>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Two new Makefile targets are added: nix-run executes the CLI via Nix, and update-nix-vendor-hash automates vendor hash updates in pkgs/defang/cli.nix. The test-nix flow is modified to attempt nix-run first, then trigger the hash update target on failure.

Changes

Cohort / File(s) Change Summary
Makefile automation targets
Makefile
Added nix-run target to execute CLI via Nix with flakes and nix-command features. Added update-nix-vendor-hash target to compute new vendor hash, extract sha256, update pkgs/defang/cli.nix, stage the file, and exit with failure. Modified test-nix target to invoke nix-run, and if it fails, trigger vendor hash remediation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify the hash extraction and update logic in update-nix-vendor-hash correctly parses and stages the vendor hash
  • Confirm the fallback flow in test-nix properly triggers the remediation path on failure
  • Check that exit codes are correct to prevent silent failures and ensure proper error propagation

Possibly related issues

Poem

🐰 A rabbit hops through Nix with glee,
New targets built for harmony,
When hashes fail, a path is found,
Automation spins with magic round!
🔧✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'automatically update the nix vendor hash' directly describes the main change: adding automation to update the Nix vendor hash, which matches the PR's core objective of reducing CI churn.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jordan/automatically-update-nix-vendor-hash

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (1)
Makefile (1)

39-46: Harden update-nix-vendor-hash: pipe failures, magic hash, and staging on error

The overall flow is good, but this target is brittle in a few ways:

  • The nix-run | grep | tail | xargs sed pipeline ignores failures (no set -o pipefail), so if grep finds nothing or nix-run fails differently, the recipe still progresses to git add with an unchanged or invalid hash.
  • The placeholder 0AAAAAAAA… is a magic value with no explanation; it’s non-obvious why that particular string is used to force Nix to recompute the vendor hash.
  • sed -i.bak is run twice, both times writing the same .bak file, which is then removed once. That works, but it’s a bit confusing and masks which step actually changed the file.

These issues can cause cli.nix to be staged with a still-broken vendorHash and leave the developer in a confusing state.

Consider tightening this target along these lines:

.PHONY: update-nix-vendor-hash
update-nix-vendor-hash:
-	sed -i.bak -E 's|(vendorHash = "sha256-)[A-Za-z0-9+/=]+(")|\10AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=\2|' pkgs/defang/cli.nix
-	$(MAKE) nix-run 2>&1 | grep -o 'sha256-[A-Za-z0-9+/=]\+' | tail -n1 | xargs -I {} sed -i.bak -E 's|(vendorHash = ")[^"]+(")|vendorHash = "{}"|' pkgs/defang/cli.nix
-	rm pkgs/defang/cli.nix.bak
-	git add pkgs/defang/cli.nix
-	@echo cli.nix vendorHash has been updated; commit and push
-	@false
+	@bash -euo pipefail -c '\
+	  # Force a vendorHash mismatch so Nix prints the expected one. Magic value: 44 "A"s. \
+	  sed -i .bak1 -E "s|(vendorHash = \"sha256-)[A-Za-z0-9+/=]+(\")|\1AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\2|" pkgs/defang/cli.nix; \
+	  nix run .#defang-cli --extra-experimental-features flakes --extra-experimental-features nix-command 2>&1 \
+	    | grep -o "sha256-[A-Za-z0-9+/=]\+" | tail -n1 \
+	    | xargs -r -I {} sed -i .bak2 -E "s|(vendorHash = \")[^\"]+(\" )|vendorHash = \"{}\" \2|" pkgs/defang/cli.nix; \
+	'
+	@rm -f pkgs/defang/cli.nix.bak1 pkgs/defang/cli.nix.bak2
+	git add pkgs/defang/cli.nix
+	@echo cli.nix vendorHash has been updated; commit and push
+	@false

Key points: explicit comment for the magic hash, set -euo pipefail for the pipeline, and only staging the file after a fully successful update.

🧹 Nitpick comments (1)
Makefile (1)

33-36: test-nix logic is correct; consider simplifying the shell flow

The conditional logic works as intended: only when nix-run fails do we attempt update-nix-vendor-hash. You can simplify and avoid the manual $? check with a more idiomatic one-liner:

-test-nix:
-ifneq (,$(shell which nix))
-	+$(MAKE) nix-run; \
-	if [ $$? -ne 0 ]; then \
-		$(MAKE) update-nix-vendor-hash; \
-	fi
-endif
+test-nix:
+ifneq (,$(shell which nix))
+	+$(MAKE) nix-run || $(MAKE) update-nix-vendor-hash
+endif

This keeps behavior the same while reducing shell boilerplate.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 17af806 and fa8ea22.

📒 Files selected for processing (1)
  • Makefile (1 hunks)
⏰ 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: Analyze (go)
  • GitHub Check: go-test
🔇 Additional comments (1)
Makefile (1)

48-50: nix-run target looks good and keeps the Nix invocation centralized

Centralizing the nix run invocation behind nix-run is clean and makes reuse easy from other targets. No issues from a Make/Nix perspective.

Copy link
Member

@lionello lionello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway we can test this before merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants