feat: replace docker setup with arkade-regtest submodule#168
feat: replace docker setup with arkade-regtest submodule#168Dunsin-cyber wants to merge 4 commits into
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe pull request migrates the regtest environment setup from a ChangesRegtest Infrastructure Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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: 2
🧹 Nitpick comments (1)
Makefile (1)
50-60: ⚡ Quick winAdd a preflight guard for missing
regtestscripts.Direct script invocation is fine, but a missing/uninitialized submodule gives a cryptic failure. Add a quick existence check for clearer errors.
Suggested change
regtest: `@echo` "Starting regtest..." + `@test` -x ./regtest/start-env.sh || (echo "regtest submodule is missing. Run: git submodule update --init --recursive" && exit 1) @./regtest/start-env.sh regtestdown: `@echo` "Stopping regtest..." + `@test` -x ./regtest/stop-env.sh || (echo "regtest submodule is missing. Run: git submodule update --init --recursive" && exit 1) @./regtest/stop-env.sh regtestclean: `@echo` "Cleaning regtest..." + `@test` -x ./regtest/clean-env.sh || (echo "regtest submodule is missing. Run: git submodule update --init --recursive" && exit 1) @./regtest/clean-env.sh🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 50 - 60, Make each regtest target (regtest, regtestdown, regtestclean) first check that the referenced script exists and is executable (e.g., verify ./regtest/start-env.sh, ./regtest/stop-env.sh, ./regtest/clean-env.sh) and print a clear error and exit non‑zero if missing; update the targets to run a short shell guard (test -x or [ -f ... ] && ...) before invoking the script so missing/uninitialized submodules produce an explicit message and fail early instead of a cryptic error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.regtest:
- Around line 1-2: The ARKD_IMAGE and ARKD_WALLET_IMAGE entries currently use
floating "latest" tags making regtest non-deterministic; replace both with
immutable references (specific version tags like vX.Y.Z or image digests e.g.
`@sha256`:...) for ARKD_IMAGE and ARKD_WALLET_IMAGE in .env.regtest, updating each
value to the chosen tag/digest (obtain the exact digest or stable tag from
ghcr.io/arkade-os/arkd and ghcr.io/arkade-os/arkd-wallet manifests) so the
regtest environment is reproducible.
In @.github/workflows/integration.yaml:
- Around line 27-32: The cache key in the "Cache nigiri build" step currently
uses only hashFiles('regtest/.env.defaults', '.env.regtest') and therefore
misses changes to the regtest repository content; add a step to capture the
regtest revision (e.g., run git -C regtest rev-parse HEAD and export it to
GITHUB_ENV or an output) and include that revision in the cache key (replace
key: nigiri-${{ hashFiles(...) }} with something like nigiri-${{ env.REGTEST_REV
}}-${{ hashFiles(...) }}) so the cache invalidates when regtest code changes;
update references to the key string and ensure the git command step runs before
the "Cache nigiri build" step.
---
Nitpick comments:
In `@Makefile`:
- Around line 50-60: Make each regtest target (regtest, regtestdown,
regtestclean) first check that the referenced script exists and is executable
(e.g., verify ./regtest/start-env.sh, ./regtest/stop-env.sh,
./regtest/clean-env.sh) and print a clear error and exit non‑zero if missing;
update the targets to run a short shell guard (test -x or [ -f ... ] && ...)
before invoking the script so missing/uninitialized submodules produce an
explicit message and fail early instead of a cryptic error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc49ab71-5d6e-4f39-9efd-10a609877872
📒 Files selected for processing (9)
.env.regtest.github/workflows/integration.yaml.gitmodulesMakefileregtesttest/docker/docker-compose.ymltest/docker/server.Dockerfiletest/docker/setup.gotest/docker/wallet.Dockerfile
💤 Files with no reviewable changes (4)
- test/docker/docker-compose.yml
- test/docker/setup.go
- test/docker/server.Dockerfile
- test/docker/wallet.Dockerfile
| - name: Cache nigiri build | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: regtest/_build | ||
| key: nigiri-${{ hashFiles('regtest/.env.defaults', '.env.regtest') }} | ||
|
|
There was a problem hiding this comment.
Cache key misses regtest revision changes, risking stale _build reuse.
Current key ignores submodule commit changes, so Line 30 artifacts can be restored even when regtest code changed.
Suggested change
- - name: Cache nigiri build
+ - name: Cache regtest build
uses: actions/cache@v4
with:
path: regtest/_build
- key: nigiri-${{ hashFiles('regtest/.env.defaults', '.env.regtest') }}
+ key: regtest-${{ runner.os }}-${{ github.sha }}-${{ hashFiles('regtest/.env.defaults', '.env.regtest') }}
+ restore-keys: |
+ regtest-${{ runner.os }}-🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/integration.yaml around lines 27 - 32, The cache key in
the "Cache nigiri build" step currently uses only
hashFiles('regtest/.env.defaults', '.env.regtest') and therefore misses changes
to the regtest repository content; add a step to capture the regtest revision
(e.g., run git -C regtest rev-parse HEAD and export it to GITHUB_ENV or an
output) and include that revision in the cache key (replace key: nigiri-${{
hashFiles(...) }} with something like nigiri-${{ env.REGTEST_REV }}-${{
hashFiles(...) }}) so the cache invalidates when regtest code changes; update
references to the key string and ensure the git command step runs before the
"Cache nigiri build" step.
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — feat: replace docker setup with arkade-regtest submodule
Scope: Test infrastructure only. No protocol-critical code touched — no VTXO handling, signing, forfeit paths, or exit logic changed.
✅ What looks good
-
Clean migration. Old inline docker-compose + Dockerfiles +
setup.goreplaced by the sharedarkade-regtestsubmodule. Less code to maintain, consistent across SDKs. -
Config override mechanism works correctly.
regtest/lib/env.shauto-discovers$SCRIPT_DIR/../.env.regtest, so the override file at repo root will be picked up without needing--envflag. Verified the load order:.env.defaults→.env.regtest. -
Critical timing values preserved. The
.env.regtestcorrectly overrides arkade-regtest's much larger defaults back to the values the go-sdk e2e tests expect:ARKD_VTXO_TREE_EXPIRY=20(default 1024)ARKD_UNILATERAL_EXIT_DELAY=10(default 512)ARKD_BOARDING_EXIT_DELAY=30(default 2048)ARKD_SESSION_DURATION=10(default 30)
-
Hardcoded infra vars handled by arkade-regtest.
ARKD_NO_MACAROONS,ARKD_WALLET_ADDR,ARKD_ESPLORA_URL,ARKD_VTXO_MIN_AMOUNT=1,ARKD_DB_TYPE=sqlite,ARKD_EVENT_DB_TYPE=badgerare all hardcoded indocker-compose.arkd-override.yml. No regression. -
CI improvements. Failure log capture (
docker logs arkd), explicit cleanup step, and nigiri build cache are all good additions.
⚠️ Minor issues
-
.github/workflows/integration.yaml— missing trailing newline (last line isrun: make regtestcleanwith no\n). POSIX compliance and diff noise. Same for the old file, but since you're editing it anyway, add the newline. -
ARKD_LOG_LEVELnot overridden in.env.regtest. Old setup used5(DEBUG), arkade-regtest defaults to4(INFO). Not a correctness issue, but you'll get less diagnostic output when tests fail. Consider addingARKD_LOG_LEVEL=5to.env.regtestto match old behavior. -
Pinned to submodule commit
1a10171. This is fine for reproducibility, but document somewhere (or in CI) how to bump the submodule when arkade-regtest updates. A stale pin could cause confusion later. -
CI hasn't run yet (
action_requiredon all workflow runs). The config looks correct, but the proof is in the green build. Maintainers should approve the CI run and verify tests pass before merging.
📋 Verdict
No blocking issues. This is a straightforward infra migration with no protocol impact. The config mapping is correct and complete. Ship it after addressing the trailing newline and confirming CI passes.
There was a problem hiding this comment.
Arkana Follow-up Review — new commits 043da49..742f29d
All three issues from my previous review have been addressed:
| Previous finding | Status |
|---|---|
Missing trailing newline in integration.yaml |
✅ Fixed |
ARKD_LOG_LEVEL=5 not in .env.regtest |
✅ Added |
| No documented way to bump submodule | ✅ make bump-regtest target + README section |
Additional improvements in these commits
- Images pinned to
v0.9.4(.env.regtest:1-2) — good, avoids silent breakage from:latestdrift. chmod +xon regtest scripts in CI (integration.yaml:34) — necessary since git submodules don't always preserve execute bits.- Failure log capture (
integration.yaml:40-43) — capturesarkdandark-walletlogs on test failure. Helpful for debugging. - Submodule bumped
1a10171→6333e4b— verified commit exists inArkLabsHQ/arkade-regtest(pins lnurl-server to 0.1.0).
One note
CI status for 742f29d is still action_required — maintainer needs to approve the workflow run before merge to confirm tests pass.
LGTM. No protocol-critical code involved. Ship it once CI is green. ✅
|
we're not going for this until the regtest env is polished |
2 participants
fixes #163 removes the dependency on an externally running Nigiri instance and lets arkade-regtest manage the full stack (Nigiri + arkd + wallet) in one place
Summary by CodeRabbit