docs: view documentation for new docker image#1168
Conversation
This commit modifies the Docker configuration in `compose.yaml` to change the image for the `tn-db` service and adds a health check. It also updates environment variable names to `SETUP_DB_OWNER` and `SETUP_CHAIN_ID` for consistency across various Docker Compose files. Additionally, the GitHub Actions workflow is adjusted to reflect the new image name for the Node service. These changes aim to enhance clarity and maintainability of the configuration.
This commit introduces a new documentation file, `container-image-guide.md`, detailing the quickstart process for running the TRUF.NETWORK node container using Docker Compose. The guide includes prerequisites, workspace preparation, Docker Compose configuration, service launch instructions, and optional steps for pre-populating configuration. This addition aims to enhance user onboarding and provide clear instructions for setting up the node environment.
WalkthroughStandardizes container image names to ghcr.io/.../node, renames runtime/setup env vars to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI as CI
participant Runner as Test Runner
CI->>Runner: go test -failfast -short -p 1 -timeout=15m -count=1 -tags=kwiltest ./...
alt first test fails
Runner-->>CI: exit early (failfast)
else
Runner-->>CI: continue running tests
end
sequenceDiagram
autonumber
participant Init as Migration Init
participant FS as embedded FS
participant Walker as fs.WalkDir collector
participant Sorter as Sorter
Init->>Walker: GetSeedScriptPaths()
Walker->>FS: WalkDir(root)
loop each entry
FS-->>Walker: entry path
alt entry endsWith(".sql") and not endsWith(".prod.sql")
Walker->>Walker: collect path
else
Walker->>Walker: skip
end
end
Walker->>Sorter: sort collected paths
Sorter-->>Init: return sorted []string (panic if empty)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/setup/simple_node.go (1)
65-76: Don’t remove the Docker network at the end of Setup.
defer network.Remove(ctx)tears the network down as soon as Setup returns, while containers are still running.Apply:
- defer network.Remove(ctx) // Clean up network when done + // Store and remove in Teardown to avoid breaking running containers + f.network = networkAnd add the following outside this hunk:
// In struct: type SimpleNodeFixture struct { // ... network testcontainers.Network } // In Teardown (after container terminations): if f.network != nil { _ = f.network.Remove(ctx) }deployments/Dockerfile (1)
18-18: Multi-arch build will produce a broken arm64 image (forced GOARCH=amd64).
With buildx targeting linux/amd64,linux/arm64, hardcoding GOARCH causes wrong binaries on arm64.Apply:
-FROM golang:1.24.3-alpine3.21 AS build +FROM --platform=$BUILDPLATFORM golang:1.24.3-alpine3.21 AS build +ARG TARGETOS +ARG TARGETARCH @@ -RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o /app/.build/kwild /app/cmd/kwild/main.go +RUN CGO_ENABLED=0 GOOS=$TARGETOS GOARCH=$TARGETARCH \ + go build -trimpath -ldflags "-s -w" -o /app/.build/kwild /app/cmd/kwild/main.go @@ -FROM alpine:latest +FROM --platform=$TARGETPLATFORM alpine:3.20Also consider pinning the base to a non-latest tag for reproducibility (done above).
🧹 Nitpick comments (3)
tests/setup/simple_node.go (1)
98-101: Use explicit container port with protocol in MappedPort.
Passing "8484" can be fragile; testcontainers expects "8484/tcp".Apply:
- kwildPort, err := kwildContainer.MappedPort(ctx, "8484") + kwildPort, err := kwildContainer.MappedPort(ctx, "8484/tcp")deployments/Dockerfile (1)
20-24: Remove unused busybox stage.
It isn’t referenced and adds build time.-FROM busybox:1.35.0-uclibc as busybox -# busy box will provide us with a shell to run commands in distroless - -FROM alpine:latest +FROM --platform=$TARGETPLATFORM alpine:3.20tests/extensions/tn_digest/configs/custom-entrypoint.sh (1)
9-9: Quote env expansions for safety.
Prevents word-splitting; harmless even if values have no spaces.- ./kwild setup init --chain-id ${SETUP_CHAIN_ID:-tn-digest-test} --db-owner ${SETUP_DB_OWNER} -r /root/.kwild/ + ./kwild setup init --chain-id "${SETUP_CHAIN_ID:-tn-digest-test}" --db-owner "${SETUP_DB_OWNER}" -r /root/.kwild/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/publish-node-image.yaml(1 hunks)compose.yaml(2 hunks)deployments/Dockerfile(1 hunks)docs/container-image-guide.md(1 hunks)tests/extensions/tn_cache_metrics/docker-compose.yml(1 hunks)tests/extensions/tn_digest/configs/custom-entrypoint.sh(1 hunks)tests/extensions/tn_digest/docker-compose.yml(1 hunks)tests/setup/simple_node.go(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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (7)
tests/extensions/tn_digest/docker-compose.yml (1)
28-29: Env var rename looks good and consistent with entrypoint.
The switch to SETUP_CHAIN_ID/SETUP_DB_OWNER aligns with the new setup flow.tests/setup/simple_node.go (1)
163-165: LGTM on env var renames.
Matches compose/test configs and Dockerfile.tests/extensions/tn_cache_metrics/docker-compose.yml (1)
28-29: Env var rename is correct.
Consistent with other files and the setup script.deployments/Dockerfile (1)
27-27: Comment update is accurate.
Clarifies override behavior via SETUP_DB_OWNER.compose.yaml (1)
48-52: Healthcheck endpoint inconsistent with test stacks.
Tests use /health; here it’s /api/v1/health. Confirm the correct path and standardize.If /health is the canonical endpoint, apply:
- test: [ "CMD-SHELL", "wget -q --spider http://localhost:8484/api/v1/health" ] + test: [ "CMD-SHELL", "wget -q --spider http://localhost:8484/health" ]docs/container-image-guide.md (2)
78-80: Verify health endpoint path.
Docs use /api/v1/health while test compose uses /health. Standardize after confirming the canonical endpoint.
62-67: Good: docs recommend SETUP_ variables.*
Matches the intended container contract.
Time Submission Status
|
…rk/node This commit modifies various files to replace references to the `tn-db` Docker image with `ghcr.io/trufnetwork/node`. Changes include updates in the Taskfile, Docker Compose files, CI cleanup scripts, and test scripts. These adjustments ensure consistency across the codebase and align with the new image naming convention.
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)
deployments/infra/scripts/renderer/templates/tn_db_startup.sh.tmpl (1)
55-55: Don’t swallow compose startup failures.
|| truemasks errors; systemd will report success even ifup -d --waitfails.Apply:
-START_CMD="/bin/bash -c \"docker compose -f {{ .ComposePath }} up -d --wait || true\"" +START_CMD="/bin/bash -ec \"docker compose -f {{ .ComposePath }} up -d --wait\""
🧹 Nitpick comments (5)
deployments/infra/scripts/renderer/templates/tn_db_startup.sh.tmpl (3)
63-66: Tie the unit to Docker availability.Add ordering/requirements so the unit doesn’t race Docker startup.
[Unit] Description=${SERVICE_DESC} +After=docker.service containerd.service +Requires=docker.service Restart=on-failure
53-54: Optional: rename service for clarity.
tn-db-appis legacy naming; considertn-node-appto reflect the image rename.-SERVICE_NAME=tn-db-app +SERVICE_NAME=tn-node-app -SERVICE_DESC="TN Docker Application" +SERVICE_DESC="TrufNetwork Node (Docker Compose)"
73-76: Environment injection robustness.Unquoted values can break if they contain spaces/equals. Prefer an EnvironmentFile.
If feasible, write
${SERVICE_NAME}.envwith key=value pairs and replace the loop with:-{{ range $key := .SortedEnvKeys -}} -{{ $value := index $.EnvVars $key -}} -Environment={{ $key }}={{ $value }} -{{ end }} +EnvironmentFile=/etc/systemd/system/${SERVICE_NAME}.envscripts/ci-cleanup.sh (1)
13-15: Images list: remove redundantkwildb/postgres:latest.
kwildb/postgresalready points to:latest; keeping both is duplicate work.-images=("kwildb/postgres" "ghcr.io/trufnetwork/node:local" "kwildb/postgres:latest" "kwildb/postgres:16.8-1") +images=("kwildb/postgres" "ghcr.io/trufnetwork/node:local" "kwildb/postgres:16.8-1")tests/extensions/tn_cache_metrics/docker-compose.yml (1)
75-79: Healthcheck may fail ifwgetisn’t in the image.Use
CMD-SHELL+curl -sfto avoid relying onwget.- healthcheck: - test: ["CMD", "wget", "--quiet", "--tries=1", "--spider", "http://localhost:8484/health"] + healthcheck: + test: ["CMD-SHELL", "curl -sf http://localhost:8484/health >/dev/null || exit 1"] interval: 10s timeout: 5s retries: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Taskfile.yml(2 hunks)deployments/infra/scripts/renderer/templates/tn_db_startup.sh.tmpl(1 hunks)deployments/infra/scripts/renderer/testdata/TestGoldenTnScript.golden(1 hunks)docs/container-image-guide.md(1 hunks)scripts/ci-cleanup.sh(1 hunks)tests/extensions/tn_cache_metrics/docker-compose.yml(3 hunks)tests/extensions/tn_cache_metrics/test_tn_cache_metrics.sh(2 hunks)tests/extensions/tn_digest/docker-compose.yml(3 hunks)tests/extensions/tn_digest/test_tn_digest.sh(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Taskfile.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/extensions/tn_digest/docker-compose.yml
- docs/container-image-guide.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: outerlook
PR: trufnetwork/node#1168
File: .github/workflows/publish-node-image.yaml:19-19
Timestamp: 2025-09-19T18:14:01.417Z
Learning: The ghcr.io/trufnetwork/tn-db image was never released, so no backwards compatibility or transitional measures are needed when renaming to ghcr.io/trufnetwork/node.
📚 Learning: 2025-09-19T18:14:01.417Z
Learnt from: outerlook
PR: trufnetwork/node#1168
File: .github/workflows/publish-node-image.yaml:19-19
Timestamp: 2025-09-19T18:14:01.417Z
Learning: The ghcr.io/trufnetwork/tn-db image was never released, so no backwards compatibility or transitional measures are needed when renaming to ghcr.io/trufnetwork/node.
Applied to files:
deployments/infra/scripts/renderer/templates/tn_db_startup.sh.tmpldeployments/infra/scripts/renderer/testdata/TestGoldenTnScript.goldentests/extensions/tn_cache_metrics/test_tn_cache_metrics.shscripts/ci-cleanup.shtests/extensions/tn_digest/test_tn_digest.sh
🧬 Code graph analysis (1)
tests/extensions/tn_digest/test_tn_digest.sh (1)
tests/extensions/tn_cache_metrics/test_tn_cache_metrics.sh (1)
main(156-196)
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (4)
deployments/infra/scripts/renderer/testdata/TestGoldenTnScript.golden (1)
17-18: Golden updated to ghcr tag — OK.Stays in lockstep with the template change.
tests/extensions/tn_cache_metrics/docker-compose.yml (1)
19-19: Image and env key renames look consistent.
ghcr.io/trufnetwork/node:metrics-testplusSETUP_CHAIN_ID/SETUP_DB_OWNERalign with the new conventions.Also applies to: 28-29
tests/extensions/tn_cache_metrics/test_tn_cache_metrics.sh (1)
15-15: Test image tag updated — OK.Local build tags to the same name the compose uses; avoids unintended pulls.
deployments/infra/scripts/renderer/templates/tn_db_startup.sh.tmpl (1)
40-41: Approve: retag to ghcr.io/trufnetwork/node:local is correct.compose.yaml, Taskfile.yml and scripts/ci-cleanup.sh reference ghcr.io/trufnetwork/node:local; remaining "tn-db" matches are service/volume/backend names (compose service, data-tn-db, dev-gateway BACKENDS), not image tags — no ghcr.io/trufnetwork/tn-db image references found.
outerlook
changed the title
This commit modifies the `compose.yaml`, `Taskfile.yml`, and `devnet-compose.yaml` files to standardize the use of `SETUP_DB_OWNER` and `SETUP_CHAIN_ID` environment variables. The changes ensure that these variables are consistently defined and utilized across different services and tasks, enhancing clarity and maintainability. Additionally, the documentation in `container-image-guide.md` is updated for improved clarity regarding the configuration process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
deployments/dev-net/devnet-compose.yaml (1)
79-79: Inconsistent default for SETUP_DB_OWNER vs compose.yaml; consider empty-by-default.Here defaulting to a concrete address can cause accidental misuse, while compose.yaml defaults to empty. Recommend aligning to empty default for safety.
- - SETUP_DB_OWNER=${SETUP_DB_OWNER:-0x4710A8D8F0D845da110086812a32De6d90d7ff5C} + - SETUP_DB_OWNER=${SETUP_DB_OWNER:-}Also applies to: 110-110
compose.yaml (2)
31-31: Clarify intent of using both image and build.If you intend to pull and run the published image, drop the build section. If you intend to build locally, keeping both is fine but document that
docker compose buildis required and that the local build will tag ghcr.io/trufnetwork/node:local.- image: "ghcr.io/trufnetwork/node:local" + image: "ghcr.io/trufnetwork/node:local" # If building locally, keep; otherwise remove the build block below.
16-16: Typo: “persis data” → “persist data”.- # persis data + # persist data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Taskfile.yml(6 hunks)compose.yaml(2 hunks)deployments/dev-net/devnet-compose.yaml(3 hunks)docs/container-image-guide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Taskfile.yml
- docs/container-image-guide.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: outerlook
PR: trufnetwork/node#1168
File: tests/extensions/tn_digest/test_tn_digest.sh:15-15
Timestamp: 2025-09-19T18:59:51.914Z
Learning: In Docker Compose files, service names (like "tn-db:") can remain unchanged even when the underlying image is updated (e.g., from tn-db:local to ghcr.io/trufnetwork/node:local). Service names are internal network identifiers and don't need to match the actual image names.
📚 Learning: 2025-09-19T18:59:51.914Z
Learnt from: outerlook
PR: trufnetwork/node#1168
File: tests/extensions/tn_digest/test_tn_digest.sh:15-15
Timestamp: 2025-09-19T18:59:51.914Z
Learning: In Docker Compose files, service names (like "tn-db:") can remain unchanged even when the underlying image is updated (e.g., from tn-db:local to ghcr.io/trufnetwork/node:local). Service names are internal network identifiers and don't need to match the actual image names.
Applied to files:
compose.yamldeployments/dev-net/devnet-compose.yaml
📚 Learning: 2025-09-19T18:14:01.417Z
Learnt from: outerlook
PR: trufnetwork/node#1168
File: .github/workflows/publish-node-image.yaml:19-19
Timestamp: 2025-09-19T18:14:01.417Z
Learning: The ghcr.io/trufnetwork/tn-db image was never released, so no backwards compatibility or transitional measures are needed when renaming to ghcr.io/trufnetwork/node.
Applied to files:
compose.yaml
⏰ 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: lint
- GitHub Check: acceptance-test
🔇 Additional comments (4)
deployments/dev-net/devnet-compose.yaml (2)
137-137: Top-level volume declaration for tn-conf: LGTM.This ensures the named volume exists and avoids anonymous volume churn.
79-81: SETUP_ env migration confirmed — no CHAIN_ID build-arg present*deployments/Dockerfile defines ENV SETUP_CHAIN_ID (line 25) and the setup command uses $SETUP_CHAIN_ID; no ARG CHAIN_ID or build-arg usage detected.
compose.yaml (2)
41-42: Env passthrough fixed to SETUP_ — aligns with docs/tests.*This addresses the earlier mismatch flagged in review. Thanks for tightening it up.
60-61: extra_hosts host-gateway: LGTM.Helpful for Linux; benign elsewhere.
This commit updates the `go.mod` and `go.sum` files to reference the latest versions of the `kwil-db` and `kwil-db/core` packages. The changes ensure that the project is using the most recent updates and improvements from these dependencies, enhancing overall stability and performance.
This commit updates the parameter order in the `InjectERC20Transfer` function calls within the ERC20 bridge transfer action test file. The changes ensure that the correct arguments are passed, improving the accuracy of the tests and maintaining consistency across the test cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
go.mod (1)
22-23: Docs‑only PR is carrying a dep bump; consider isolating or call it out.This change widens the blast radius for a documentation PR. Either split to a separate PR or add a short note in the description and ensure a quick smoke test passes before merge (the PR already notes waiting on issue #1169).
If keeping it here, please confirm CI covers a minimal build and the container quickstart path with these versions.
tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (2)
124-126: Same arg‑order verification and inline labels here.Mirror the clarity/refactor to prevent future regressions.
- err = testerc20.InjectERC20Transfer(ctx, platform, - TestChain, configuredEscrow, TestERC20, TestUserA, configuredEscrow, smallAmount, 10, nil) + err = testerc20.InjectERC20Transfer( + ctx, platform, + TestChain, // chain + configuredEscrow, // from + TestERC20, // token + TestUserA, // to + configuredEscrow, // escrow + smallAmount, // amount + 10, // confirmations + nil, // opts + )
173-175: Repeat: verify param order and add arg comments on the 10‑token credit.Keeps consistency with the earlier calls and reduces ambiguity.
- err = testerc20.InjectERC20Transfer(ctx, platform, - TestChain, configuredEscrow, TestERC20, userA, configuredEscrow, initialAmount, 10, nil) + err = testerc20.InjectERC20Transfer( + ctx, platform, + TestChain, // chain + configuredEscrow, // from + TestERC20, // token + userA, // to + configuredEscrow, // escrow + initialAmount, // amount + 10, // confirmations + nil, // opts + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(1 hunks)tests/extensions/erc20/erc20_bridge_transfer_actions_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: outerlook
PR: trufnetwork/node#1168
File: tests/extensions/tn_digest/test_tn_digest.sh:15-15
Timestamp: 2025-09-19T18:59:51.914Z
Learning: In Docker Compose files, service names (like "tn-db:") can remain unchanged even when the underlying image is updated (e.g., from tn-db:local to ghcr.io/trufnetwork/node:local). Service names are internal network identifiers and don't need to match the actual image names.
🧬 Code graph analysis (1)
tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (1)
tests/extensions/erc20/common_test.go (4)
TestChain(20-20)TestERC20(24-24)TestUserA(25-25)TestAmount2(28-28)
⏰ 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: acceptance-test
- GitHub Check: lint
🔇 Additional comments (3)
go.mod (1)
22-23: Approve: kwil-db pseudo-version and checksums verified.
Commit a6b854684a3e exists upstream (2025-09-16T11:52:10Z); matching go.sum entries are present and go.mod lines 22–23 are correct.tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (2)
1-10: Scope check — helper signature and usages verifiedInjectERC20Transfer is declared in tests/streams/utils/erc20/inject.go and all test call sites use the same argument order; the change is consistent and appears intentional.
33-35: Correct InjectERC20Transfer parameter ordering — annotate call siteActual signature (tests/streams/utils/erc20/inject.go):
InjectERC20Transfer(ctx, platform, chain, escrow, erc20Addr, fromHex, toHex, valueStr, point, prev)The call at tests/extensions/erc20/erc20_bridge_transfer_actions_test.go:33 passes (chain=TestChain, escrow=configuredEscrow, erc20=TestERC20, from=TestUserA, to=configuredEscrow). Annotate the call to make this explicit:
err = testerc20.InjectERC20Transfer( ctx, platform, TestChain, // chain configuredEscrow, // escrow TestERC20, // erc20Addr TestUserA, // fromHex configuredEscrow, // toHex TestAmount2, // valueStr 10, // point nil, // prev ) require.NoError(t, err)Likely an incorrect or invalid review comment.
This commit refactors the `GetSeedScriptPaths` function in `migration.go` to utilize `fs.WalkDir` for more efficient and cleaner retrieval of embedded seed files. The changes enhance the handling of SQL files by filtering out unnecessary files and ensuring that only relevant seed files are returned. Additionally, the test for the presence of seed files is updated for clarity. The CI workflow is also modified to use the `-failfast` flag in tests, improving test execution behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/migrations/migration.go (1)
42-48: Prefer returning an error over panicking (or provide a Must helper)*Panic on “no seeds” and walk errors propagates crashes into callers. Consider
GetSeedScriptPaths() ([]string, error)and a thinMustGetSeedScriptPaths()for CLI paths.- if len(seedFilesPaths) == 0 { - panic("no seeds files found in embedded directory") - } + if len(seedFilesPaths) == 0 { + panic("no seed files found in embedded FS") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yaml(1 hunks)internal/migrations/migration.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: outerlook
PR: trufnetwork/node#1168
File: tests/extensions/tn_digest/test_tn_digest.sh:15-15
Timestamp: 2025-09-19T18:59:51.914Z
Learning: In Docker Compose files, service names (like "tn-db:") can remain unchanged even when the underlying image is updated (e.g., from tn-db:local to ghcr.io/trufnetwork/node:local). Service names are internal network identifiers and don't need to match the actual image names.
📚 Learning: 2025-09-12T01:16:28.152Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: internal/migrations/migration.go:34-43
Timestamp: 2025-09-12T01:16:28.152Z
Learning: In internal/migrations/migration.go, the maintainer (outerlook) prefers fail-fast behavior when the test_only directory is missing. The code should panic rather than gracefully handle missing test directories, as this indicates a build/configuration problem that should be caught immediately.
Applied to files:
.github/workflows/ci.yaml
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (3)
.github/workflows/ci.yaml (1)
92-92: Adding -failfast is reasonable for quicker signal; confirm intended diagnostics trade-off.This aligns with the team’s fail‑fast preference from prior work and should shorten feedback loops. Note that with
-p 1, the runtime benefit is moderate and CI will stop at the first failure, potentially hiding additional failing tests until retries.Can you confirm we’re okay with reduced failure surface in PR runs, or do we want a non‑failfast path for nightly/regression jobs?
internal/migrations/migration.go (2)
5-9: Imports change looks fineAdding io/fs and sort is appropriate for WalkDir + sorting.
32-33: Make .prod.sql inclusion opt‑in — don't hard‑excludeAudit: GetSeedScriptPaths() is used widely in tests/benchmarks and there are currently no .prod.sql files in the repo; still, avoid an unconditional skip — make inclusion opt‑in via env.
- if strings.HasSuffix(path, ".prod.sql") { + includeProd := os.Getenv("TRUF_INCLUDE_PROD_SEEDS") == "1" + if strings.HasSuffix(path, ".prod.sql") && !includeProd { return nil }Location: internal/migrations/migration.go (around lines 32–33).
…nnet This commit introduces a new SQL migration file, `000-extension.prod.sql`, specifically for the mainnet configuration of the ERC20 bridge. The previous migration file, `000-extension.sql`, has been removed as it was only necessary for leader and validator nodes. The new file includes a note indicating that it should not be run in test environments, ensuring clarity in its intended use.
This commit introduces a new SQL migration file, `bootstrap_erc20.sql`, for enabling the ERC20 bootstrap process in tests, and a new `000-extension.sql` for the ERC20 bridge with a note indicating it should not be run in test environments. Additionally, the `GetSeedScriptPaths` function in `migration.go` is updated to include the new `0_test_only` directory for seed file retrieval, ensuring proper handling of embedded SQL files.
This commit adds the `erc20shim` import to the `runner.go` file and updates the `RunSchemaTest` function to call `erc20shim.ForTestingResetSingleton()` during test execution. This enhancement ensures that the ERC20 bridge's state is properly reset for each test, improving test reliability and consistency.
This commit removes unnecessary cleanup steps in the `seedAndRun` function of `common_test.go` and adds a call to `erc20shim.ForTestingClearAllInstances` in the `RunSchemaTest` function of `runner.go`. These changes enhance the efficiency of the test setup by simplifying the cleanup logic and ensuring a fresh state for each test, thereby improving test reliability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/extensions/erc20/common_test.go (2)
73-78: Unused Cleanup in TestingT here.This interface advertises Cleanup but seedAndRun no longer registers any cleanup. Either remove Cleanup from this local TestingT or use it (belt-and-suspenders) to register a shim reset after each test.
type TestingT interface { Fatalf(format string, args ...interface{}) Errorf(format string, args ...interface{}) - Cleanup(func()) }Alternatively, keep it and register a no-op or future cleanup.
35-41: Centralize ERC‑20 shim resets in the runner.No direct uses of
kwilTesting.RunSchemaTestwere found outside tests/streams/utils/runner.go; runner.go already callserc20shim.ForTestingResetSingleton()/ForTestingClearAllInstances(), but tests/streams/utils/erc20/helper.go:28 still callserc20shim.ForTestingResetSingleton(). Move that helper reset into the runner and ensure all ERC‑20 tests usetestutils.RunSchemaTest(notkwilTesting.RunSchemaTest) to avoid state leakage.tests/streams/utils/runner.go (2)
92-94: Pre-init ERC‑20 resets are correct; consider DRYing.Reset-before-init is the right spot (EngineReadyHooks timing). The same 3-line reset appears twice; extract a helper to reduce drift.
Apply:
- orderedsync.ForTestingReset() - erc20shim.ForTestingResetSingleton() - erc20shim.ForTestingClearAllInstances(context.Background(), nil) + resetTestExtensions()Add this helper (placement: below imports or near other helpers):
func resetTestExtensions() { orderedsync.ForTestingReset() erc20shim.ForTestingResetSingleton() erc20shim.ForTestingClearAllInstances(context.Background(), nil) }
106-108: Extract duplicate reset logic into resetTestExtensions()Replace the duplicated ordered-sync / ERC20 reset calls with a single helper to avoid divergence.
Location: tests/streams/utils/runner.go:91-93 & 105-107.- orderedsync.ForTestingReset() - erc20shim.ForTestingResetSingleton() - erc20shim.ForTestingClearAllInstances(context.Background(), nil) + resetTestExtensions()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/extensions/erc20/common_test.go(1 hunks)tests/streams/utils/runner.go(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-12T01:16:55.187Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/extensions/erc20/erc20_bridge_epoch_test.go:31-32
Timestamp: 2025-09-12T01:16:55.187Z
Learning: In the ERC20 bridge tests, the test environment is reset after each test via seedAndRun function and cleanup mechanisms, so using shared aliases like TestChain is acceptable and doesn't cause collisions between tests.
Applied to files:
tests/streams/utils/runner.go
📚 Learning: 2025-09-12T01:19:04.545Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/extensions/erc20/erc20_bridge_actions_smoke_test.go:40-47
Timestamp: 2025-09-12T01:19:04.545Z
Learning: In ERC20 bridge tests, the maintainer (outerlook) prefers fail-fast behavior through panics when schema changes occur. Direct indexing and type assertions in test row handlers should be maintained to immediately catch unexpected schema changes, rather than using graceful error handling.
Applied to files:
tests/streams/utils/runner.go
📚 Learning: 2025-09-12T12:03:13.782Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/streams/utils/erc20/helper.go:37-41
Timestamp: 2025-09-12T12:03:13.782Z
Learning: In the ERC20 bridge test helpers at tests/streams/utils/erc20/helper.go, the extensionAlias parameter serves dual purpose as both the extension identifier and the chain name. This is a deliberate design choice in their test setup where extensionAlias and chain are the same value.
Applied to files:
tests/streams/utils/runner.go
⏰ 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: lint
- GitHub Check: acceptance-test
🔇 Additional comments (1)
tests/streams/utils/runner.go (1)
20-20: Import alias LGTM.Bringing in erc20shim explicitly clarifies ownership and keeps test resets colocated with ordered-sync resets.
This commit modifies the `testListMetadataByHeight` function in `metadata_test.go` to dynamically set the `Height` parameter for metadata insertion and retrieval based on the index of the item in the `metadataItems` slice. This change ensures that the height values are correctly assigned, improving the accuracy of the test results.
This commit updates the `testListMetadataByHeight` function in `metadata_test.go` to include an additional metadata item with a height of "2" and modifies the height assignment logic to convert the item's value to an integer. These changes improve the accuracy of the test results by ensuring that the height values are correctly represented during metadata insertion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/streams/query/metadata_test.go (3)
186-197: Parse height as int64 directly and improve error context.Avoid int→int64 hop and include the offending value in the error.
Apply this diff:
- height, err := strconv.Atoi(item.Value) - if err != nil { - return errors.Wrapf(err, "error converting value to int") - } + h, err := strconv.ParseInt(item.Value, 10, 64) + if err != nil { + return errors.Wrapf(err, "error converting value to int64: value=%q", item.Value) + } err = procedure.InsertMetadata(ctx, procedure.InsertMetadataInput{ Platform: platform, Locator: contractInfo.Locator, Key: item.Key, Value: item.Value, ValType: item.ValType, - Height: int64(height), + Height: h, })
203-207: Don’t couple target height to list length; use actual max inserted height.Safer if test data changes (e.g., non-contiguous heights).
Apply this diff:
- Height: int64(len(metadataItems)), + Height: maxHeight,And add/update in the loop section:
// before the loop var maxHeight int64 // inside the loop after parsing `h` if h > maxHeight { maxHeight = h }
212-217: Expected table aligns with ascending order; OK if API guarantees it.If ordering isn’t guaranteed, assert it explicitly or sort before comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/streams/query/metadata_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/streams/query/metadata_test.go (1)
tests/streams/utils/procedure/metadata.go (2)
InsertMetadata(293-328)InsertMetadataInput(283-290)
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (2)
tests/streams/query/metadata_test.go (2)
6-6: Import looks good.Needed for the new height parsing.
181-183: Resolved — list_metadata_by_height already orders by created_at ASC.
internal/migrations/021-metadata-actions.sql containsORDER BY m.created_at ASC, so deterministic ascending ordering by height is guaranteed; no change required.
MicBun
changed the title
2 participants
Description
Related Problem
How Has This Been Tested?
Summary by CodeRabbit
New Features
Documentation
Chores
Tests