feat: add nats hord driver support #191
Conversation
This commit introduces support for using NATS as a key-value store via the Hord library. NATS configuration and testing align with established patterns within the Tarmac project. The NATS test case in TestFullService is treated consistently with other integration tests. Unnecessary inline comments added during development have been removed. Changes include: - NATS configuration parameters are fetched directly (e.g., `nats_urls`) in `pkg/app/app.go`. - The `pkg/config/config.go` file no longer contains NATS-specific structures. - Documentation in `docs/running-tarmac/configuration.md` has been updated for the corrected configuration keys. - NATS testing is integrated into the `TestFullService` function in `pkg/app/server_test.go`. - The import alias for the NATS hord driver is now `nats`. - Unnecessary inline comments related to NATS have been removed from `pkg/app/app.go`, `pkg/app/app_test.go`, and `pkg/app/server_test.go`. - The CI configuration in `.github/workflows/build.yml` includes a job for NATS tests, running a NATS server.
WalkthroughSupport for using NATS as a key-value store backend has been added to the application. This includes updates to documentation, configuration handling, and automated tests, as well as a new GitHub Actions workflow job that runs tests against a local NATS service. No changes were made to exported or public entity declarations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant NATS
User->>Server: Start with kvstore_type = "nats"
Server->>NATS: Connect using config (servers, user, password)
alt Connection successful
Server->>User: Server runs with NATS as KV store
else Connection fails
Server->>User: Error: failed to connect to NATS kvstore
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 (6)
docs/running-tarmac/kvstore.md (1)
22-23: Row ordering is inconsistent with the existing alphabetical patternPrevious rows are roughly ordered alphabetically by the Type option (
boltdb,cassandra,in-memory,redis).
Addingnatsaftercassandrabreaks that pattern and makes scanning harder.-| Cassandra | `cassandra` | Cassandra including TLS connectivity | Eventual Consistency, Persistent Storage, Large sets of data | -| NATS | `nats` | NATS Server including JetStream for KV | Flexible, Cloud-Native Messaging and KV, Eventual Consistency | +| Cassandra | `cassandra` | Cassandra including TLS connectivity | Eventual Consistency, Persistent Storage, Large sets of data | +| NATS | `nats` | NATS Server including JetStream for KV | Flexible, Cloud-Native Messaging and KV, Eventual Consistency | +| Redis | `redis` | Redis including Sentinel and Enterprise capabilities | Strong Consistency, Fast Reads and Writes, Non-Persistent storage |(Re-ordering only—content unchanged.)
docs/running-tarmac/configuration.md (1)
33-36: Missing defaults / validation guidance for new NATS varsFor every other backend we document whether a variable is required or has a default.
Consider explicitly stating:
- default value when
kvstore_type != nats(empty string?)- behaviour when
APP_NATS_USER/PASSWORDare omitted but the server requires auth.This keeps the docs self-contained and avoids guessing.
pkg/app/app.go (1)
31-32: Import alias shadows common package nameUsing the bare identifier
natsfor the driver means later code refers tonats.Dial, which can be confused with the upstreamgithub.com/nats-io/nats.go.
To avoid ambiguity, import with an alias:- "github.com/tarmac-project/hord/drivers/nats" + natsdrv "github.com/tarmac-project/hord/drivers/nats"And update the call-site accordingly.
pkg/app/app_test.go (1)
89-97: Test name nit & missing negative assertionNice addition.
Minor: all other keys are lower-case (“invalid kvstore address”), but this one is titled “invalid NATS Address”. Consider consistent casing.Optional: assert that the returned error string contains “nats” to ensure we’re failing for the expected reason, not something unrelated (e.g. port clash).
pkg/app/server_test.go (1)
222-230: Port reuse may cause flaky testsEvery full-service sub-test binds to
localhost:9001.
If the previous server hasn’t released the socket yet, the NATS case (or vice-versa) can fail with “address already in use”, especially on CI with slower teardown.Recommend allocating a unique free port per test:
port, _ := freeport.Get() cfg.Set("listen_addr", fmt.Sprintf("localhost:%d", port))(Or bump to 0 and read back
srv.httpServer.AddrafterListen)..github/workflows/build.yml (1)
1-2: Lock down GITHUB_TOKEN permissions
Define an explicit, minimal permissions block to follow least privilege.Apply this diff:
name: build +permissions: + contents: read on:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build.yml(1 hunks)docs/running-tarmac/configuration.md(1 hunks)docs/running-tarmac/kvstore.md(1 hunks)pkg/app/app.go(2 hunks)pkg/app/app_test.go(1 hunks)pkg/app/server_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/build.yml
[warning] 123-142: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 YAMLlint (1.37.1)
.github/workflows/build.yml
[warning] 130-130: wrong indentation: expected 6 but found 4
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: nats
- GitHub Check: postgres
- GitHub Check: boltdb
- GitHub Check: mysql
- GitHub Check: in-memory
- GitHub Check: redis
- GitHub Check: cassandra
- GitHub Check: tests
- GitHub Check: build
- GitHub Check: golangci
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
.github/workflows/build.yml (2)
121-142: Add NATS integration tests job
The newnatsjob mirrors existing KV-store jobs, setting up the NATS service container and invokingmake build tests-nats.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 123-142: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}🪛 YAMLlint (1.37.1)
[warning] 130-130: wrong indentation: expected 6 but found 4
(indentation)
130-130: Ignore YAMLlint indentation warning
The 4-space indent for- uses:is consistent with existing jobs; you can safely disregard this lint alert.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 130-130: wrong indentation: expected 6 but found 4
(indentation)
pkg/app/app.go
Outdated
| case "nats": | ||
| srv.kv, err = nats.Dial(nats.Config{ | ||
| Servers: srv.cfg.GetString("nats_urls"), | ||
| User: srv.cfg.GetString("nats_user"), | ||
| Password: srv.cfg.GetString("nats_password"), | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("could not establish nats kvstore connection - %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
No empty-config guard & multi-URL handling for NATS connection
- The code proceeds to dial even when
nats_urlsis empty, returning a generic “unknown host” later. - Docs say the value is “comma-separated list”, but the field is forwarded verbatim; if
nats.Dialexpects a slice you’ll only ever use the first URL.
- case "nats":
- srv.kv, err = nats.Dial(nats.Config{
- Servers: srv.cfg.GetString("nats_urls"),
- User: srv.cfg.GetString("nats_user"),
- Password: srv.cfg.GetString("nats_password"),
- })
+ case "nats":
+ rawURLs := strings.TrimSpace(srv.cfg.GetString("nats_urls"))
+ if rawURLs == "" {
+ return fmt.Errorf("nats_urls must be provided when kvstore_type is nats")
+ }
+
+ urls := strings.Split(rawURLs, ",")
+
+ srv.kv, err = natsdrv.Dial(natsdrv.Config{
+ Servers: urls,
+ User: srv.cfg.GetString("nats_user"),
+ Password: srv.cfg.GetString("nats_password"),
+ })Also remember to import "strings" and to use the alias suggested above.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/app/app.go around lines 325 to 333, add a guard to check if the
"nats_urls" config value is empty before attempting to dial, returning an error
early if so. Then, split the "nats_urls" string by commas into a slice to handle
multiple URLs properly, and pass this slice to the nats.Dial configuration
instead of the raw string. Also, ensure the "strings" package is imported and
use the suggested alias for it in the file.
.github/workflows/build.yml
Outdated
|
|
||
| nats: | ||
| runs-on: ubuntu-latest | ||
| services: | ||
| nats: | ||
| image: nats:latest | ||
| ports: | ||
| - 4222:4222 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.21' | ||
| - name: Execute Tests | ||
| env: | ||
| NATS_URLS: nats://localhost:4222 # Ensure test uses this if it becomes configurable | ||
| run: make build tests-nats # Assuming a make target 'tests-nats' will be created | ||
| - name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Makefile target for NATS tests
Ensure the tests-nats make target exists to avoid failures when this workflow runs.
🏁 Script executed:
#!/bin/bash
# Check for tests-nats target in Makefile
grep -R "^tests-nats:" -n MakefileLength of output: 36
I’ll check the entire repo for any tests-nats references and locate all Makefile files to ensure we’re not missing a secondary Makefile.
🏁 Script executed:
#!/bin/bash
# Search for any definition or usage of tests-nats
grep -R "tests-nats" -n .
# List all Makefile variants in the repo
find . -type f \( -iname "Makefile" -o -iname "makefile" -o -iname "GNUmakefile" \)Length of output: 638
Add missing tests-nats Makefile target
The workflow invokes make build tests-nats but no such target exists in the root Makefile, causing CI failures.
Points to address:
.github/workflows/build.yml(lines ~136–142) callsmake build tests-nats- Root
Makefileis missing atests-nats:definition
Suggested fix:
In your root Makefile, add something like:
tests-nats:
go test ./... -tags natsOr update the workflow to call an existing test target instead of tests-nats.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 123-142: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 YAMLlint (1.37.1)
[warning] 130-130: wrong indentation: expected 6 but found 4
(indentation)
🤖 Prompt for AI Agents
In .github/workflows/build.yml around lines 121 to 142, the workflow runs `make
build tests-nats` but the root Makefile lacks a `tests-nats` target, causing CI
failures. To fix this, add a `tests-nats` target in the root Makefile that runs
the NATS-specific tests, for example by adding a target that executes `go test
./... -tags nats`. Alternatively, update the workflow to call an existing test
target if appropriate.
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
127-128: Pin NATS service container to a fixed version
Usingnats:latestin CI can lead to non-deterministic builds as upstream images change. Consider specifying a known version tag, e.g.,nats:2.11.4.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/build.yml
[warning] 132-132: wrong indentation: expected 6 but found 4
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Analyze (go)
- GitHub Check: mysql
- GitHub Check: postgres
- GitHub Check: nats
- GitHub Check: in-memory
- GitHub Check: tests
- GitHub Check: cassandra
- GitHub Check: boltdb
- GitHub Check: redis
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/build.yml (1)
11-12: Add minimal permissions for GITHUB_TOKEN
Good to seepermissions: contents: readadded to restrict default token access.
.github/workflows/build.yml
Outdated
| - name: Execute Tests | ||
| env: | ||
| NATS_URLS: nats://localhost:4222 # Ensure test uses this if it becomes configurable | ||
| run: make build tests-nats # Assuming a make target 'tests-nats' will be created |
There was a problem hiding this comment.
Ensure tests-nats make target exists
The workflow runs make build tests-nats but the root Makefile currently doesn't define a tests-nats target, which will cause CI failures. Please add a target like:
tests-nats:
go test ./... -tags natsor update the workflow to call an existing test target.
🤖 Prompt for AI Agents
In .github/workflows/build.yml around lines 137 to 140, the workflow runs `make
build tests-nats` but the Makefile lacks a `tests-nats` target, causing CI
failures. To fix this, add a `tests-nats` target in the root Makefile that runs
the tests with the nats build tag, for example: `tests-nats: go test ./... -tags
nats`. Alternatively, update the workflow to use an existing test target if
appropriate.
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
Fix YAML indentation for steps list in the nats job
List items under steps: must be indented two spaces further (6 spaces total) to be valid YAML.
Apply this diff to correct the indentation:
- steps:
- - uses: actions/checkout@v4
+ steps:
+ - uses: actions/checkout@v4And similarly adjust the indent for all subsequent - name entries.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| steps: | |
| - uses: actions/checkout@v4 | |
| steps: | |
| - uses: actions/checkout@v4 |
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 132-132: wrong indentation: expected 6 but found 4
(indentation)
🤖 Prompt for AI Agents
In .github/workflows/build.yml at lines 131 to 132, the list items under the
steps key in the nats job are not indented correctly. Fix the YAML by indenting
all list items under steps by two additional spaces (total of 6 spaces) to
ensure valid YAML syntax. Apply this indentation adjustment consistently to all
subsequent - name entries within the steps list.
This commit introduces support for using NATS as a key-value store via the Hord library. NATS configuration and testing align with established patterns within the Tarmac project. Integration tests for NATS are managed via Docker Compose, consistent with other services. The explicit sleep in the Makefile for NATS tests has been removed. Key changes include: - NATS configuration parameters are fetched directly (e.g., `nats_urls`) in `pkg/app/app.go`. - `pkg/config/config.go` no longer contains NATS-specific structures. - Documentation in `docs/running-tarmac/configuration.md` updated. - NATS testing integrated into `TestFullService` in `pkg/app/server_test.go`. - Import alias for NATS hord driver is now `nats`. - Unnecessary inline comments removed from relevant Go files. - NATS service added to `dev-compose.yml`. - `tests-nats` Makefile target updated to use Docker Compose for starting NATS and running tests via a dedicated test runner service; explicit sleep removed. - NATS service definition removed from `.github/workflows/build.yml`; CI now relies on the `make tests-nats` target. - `go.mod` and `go.sum` files are up to date.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.21' | ||
| - name: Execute Tests | ||
| run: make build tests-nats | ||
| - name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the issue, we will add a permissions block at the root of the workflow file. This block will apply to all jobs in the workflow unless overridden by job-specific permissions blocks. Based on the workflow's tasks (e.g., checking out code, running tests, and uploading coverage), the minimal required permissions are contents: read and actions: write (for uploading coverage). This ensures that the workflow has only the access it needs.
| @@ -10,2 +10,5 @@ | ||
| pull_request: | ||
| permissions: | ||
| contents: read | ||
| actions: write | ||
| jobs: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
122-135:⚠️ Potential issueAdd explicit permissions for the
natsjob.CodeQL flagged that the job does not limit
GITHUB_TOKENpermissions. Add a minimalpermissionsblock undernats:(or globally) to restrict scope.nats: + permissions: + contents: read runs-on: ubuntu-latest steps:🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 123-135: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}🪛 YAMLlint (1.37.1)
[warning] 125-125: wrong indentation: expected 6 but found 4
(indentation)
🧹 Nitpick comments (4)
.github/workflows/build.yml (1)
124-126: Fix YAML indentation for steps list.List items under
steps:must be indented two additional spaces (6 spaces total) to be valid YAML.- steps: - - uses: actions/checkout@v4 + steps: + - uses: actions/checkout@v4…and adjust all subsequent
- name/- runentries accordingly.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 125-125: wrong indentation: expected 6 but found 4
(indentation)
dev-compose.yml (3)
258-262: Prefer usingexposefor internal-only ports.Since NATS is only consumed by other services in the Compose network, switch from
portstoexposeto match the style of other internal services.- nats: - image: nats:latest - ports: - - "4222:4222" + nats: + image: nats:latest + expose: + - "4222"
263-271: Align test-runner-runpattern with other services.Other test runners append
.*to match all relevant subtests. Add the wildcard for consistency.- entrypoint: go test -v -race -covermode=atomic -coverprofile=/tmp/coverage/coverage.out -run "TestFullService/NATS KV Full Service" ./... + entrypoint: go test -v -race -covermode=atomic -coverprofile=/tmp/coverage/coverage.out -run "TestFullService/NATS KV Full Service.*" ./...
272-278: Remove redundant inline comments inenvironment.Inline comments after each env var are unnecessary and clutter the YAML. Removing them improves readability.
- - "APP_ENABLE_TLS=false" # Common in other test services + - "APP_ENABLE_TLS=false"…and apply the same cleanup to the other entries under
environment.
📜 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 (4)
.github/workflows/build.yml(1 hunks)Makefile(1 hunks)dev-compose.yml(1 hunks)go.mod(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/build.yml
[warning] 123-135: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 YAMLlint (1.37.1)
.github/workflows/build.yml
[warning] 125-125: wrong indentation: expected 6 but found 4
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: postgres
- GitHub Check: in-memory
- GitHub Check: mysql
- GitHub Check: nats
- GitHub Check: cassandra
- GitHub Check: boltdb
- GitHub Check: tests
- GitHub Check: redis
- GitHub Check: build
- GitHub Check: golangci
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
Makefile (1)
60-65: Makefile target fortests-natsis correctly defined.The new
tests-natstarget mirrors existing patterns: detaching core services, invoking the test runner, and bringing services down.
This commit introduces support for using NATS as a key-value store via the Hord library. NATS configuration and testing align with established patterns within the Tarmac project. Integration tests for NATS are now managed via Docker Compose, consistent with other services. Key changes include: - NATS configuration parameters are fetched directly (e.g., `nats_urls`) in `pkg/app/app.go`. - `pkg/config/config.go` no longer contains NATS-specific structures. - Documentation in `docs/running-tarmac/configuration.md` updated. - NATS testing integrated into `TestFullService` in `pkg/app/server_test.go`. - Import alias for NATS hord driver is now `nats`. - Unnecessary inline comments removed from relevant Go files. - NATS service added to `dev-compose.yml`. - `tests-nats` Makefile target updated to use Docker Compose for starting NATS and running tests via a dedicated test runner service. - NATS service definition removed from `.github/workflows/build.yml`. - `go.mod` and `go.sum` files are up to date.
madflojo
changed the title
This commit corrects the NATS Hord driver integration to use a single
`nats_url` configuration parameter, aligning with the driver's primary
connection method and simplifying setup for the most common use case.
Previous implementations incorrectly focused on a list of servers.
Key changes:
- Modified `pkg/app/app.go` to use `srv.cfg.GetString("nats_url")` and
populate the `URL` field of `nats.Config`. Logic for parsing
comma-separated URLs into `Servers` has been removed.
- Updated documentation in `docs/running-tarmac/configuration.md` to
reflect `nats_url` (singular) and `APP_NATS_URL` (singular),
removing references to `nats_urls` (plural).
- Test configurations in `pkg/app/server_test.go` and
`pkg/app/app_test.go` updated to use `nats_url` (singular).
- `go.mod` and `go.sum` files are confirmed to be up-to-date.
- All other aspects of the NATS integration (bucket requirement,
testing via Docker Compose, Makefile targets, CI workflow) remain
as established in previous refinements.
This commit introduces support for using NATS as a key-value store via the Hord library.
NATS configuration and testing align with established patterns within the Tarmac project. The NATS test case in TestFullService is treated consistently with other integration tests. Unnecessary inline comments added during development have been removed.
Changes include:
nats_urls) inpkg/app/app.go.pkg/config/config.gofile no longer contains NATS-specific structures.docs/running-tarmac/configuration.mdhas been updated for the corrected configuration keys.TestFullServicefunction inpkg/app/server_test.go.nats.pkg/app/app.go,pkg/app/app_test.go, andpkg/app/server_test.go..github/workflows/build.ymlincludes a job for NATS tests, running a NATS server.Summary by CodeRabbit