chore: use nix just #119
chore: use nix just #119
Conversation
WalkthroughThis update introduces a Nix flake-based development environment, adds a Justfile for task automation, and simplifies the Earthfile by removing operator-related logic and auxiliary targets. The Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Shell
participant Justfile
participant TestSuite
Developer->>Shell: Enter directory (.envrc triggers)
Shell->>Shell: Setup environment with Nix flake (impure mode)
Developer->>Justfile: just pre-commit
Justfile->>Justfile: Run generate, tidy, lint
Developer->>Justfile: just tests-all
Justfile->>TestSuite: Run unit and integration tests
TestSuite->>TestSuite: go:generate clones operator CRDs
TestSuite->>TestSuite: Run tests using updated CRDs
Possibly related PRs
Suggested reviewers
Poem
✨ 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. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
internal/grpcclient/connection_generated.go (1)
1-153: 💡 Verification agent🧩 Analysis chain
Verify mock generation is included in CI
Since this is an auto-generated file, ensure that mock generation is part of your CI process or that developers know to run the generate task when interface changes occur.
🏁 Script executed:
#!/bin/bash # Check if the Justfile's generate task includes go generate for mocks echo "Verifying that the generate task in Justfile includes commands to generate mocks" grep -A 5 "generate:" Justfile echo "Checking if there are any go:generate directives in the connection_adapter.go file" grep -A 3 "//go:generate" internal/grpcclient/connection_adapter.go || echo "No go:generate directives found"Length of output: 920
Ensure mock generation runs in CI
Verified that:
- Your Justfile’s
generatetask invokesgo generate ./…, covering all//go:generate mockgen…directives.internal/grpcclient/connection_adapter.goincludes the necessary//go:generate mockgencomments for bothConnectionandConnectionAdapter.Please update your CI pipeline to run the generate task (e.g.
just generateorgo generate ./…) on every build to guarantee mocks stay in sync when these interfaces change.
🧹 Nitpick comments (2)
flake.nix (1)
51-67: Dev-shell package list contains duplicates and potentially non-redistributable software
•gotoolsandgo-toolsare synonyms – include one.
•pkgs.nur.repos.goreleaser.goreleaser-prois the commercial edition; it requires a licence and will 404 for public users.
•nodejs_22may not yet be packaged; prefernodejs_20(current LTS) to avoid breakage.Recommended tidy-up:
- gotools - go-tools + gotools # only once ... - pkgs.nur.repos.goreleaser.goreleaser-pro + goreleaser ... - nodejs_22 + nodejs_20Justfile (1)
27-28: Consider adding race detection to unit testsThe unit tests task looks good, but adding the
-raceflag could help detect potential race conditions.- go test ./internal/... + go test -race ./internal/...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.github/actions/default/action.ymlis excluded by!**/*.yml.github/workflows/main.ymlis excluded by!**/*.yml.github/workflows/releases.ymlis excluded by!**/*.ymlflake.lockis excluded by!**/*.lock,!**/*.lockgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (10)
.envrc(1 hunks).gitignore(1 hunks)Earthfile(3 hunks)Justfile(1 hunks)flake.nix(1 hunks)internal/grpcclient/connection_adapter.go(1 hunks)internal/grpcclient/connection_generated.go(1 hunks)internal/membership_client_generated.go(2 hunks)internal/membership_listener.go(1 hunks)tests/main_test.go(2 hunks)
🔇 Additional comments (20)
.gitignore (1)
10-10: Add newline consistency
The newline at the end of.gitignoreensures POSIX compliance and avoids diff noise.internal/membership_listener.go (1)
35-35: Relative path ingo:generateis correct
The directive now uses a relative source and destination, aligning with project conventions. No further action needed..envrc (1)
1-1: Enable Nix flake environment
Introduceduse flake --impurefor direnv integration with the newflake.nix. This streamlines setting up the development environment.internal/membership_client_generated.go (3)
2-2: Updated source header
The generated code now references the relative sourcemembership_listener.go. Auto-generated file—skipping detailed review.
6-6: Updated generate command in documentation
Reflects the new relative paths in the mockgen invocation. Auto-generated file—skipping detailed review.
23-23: New unexported field in mock struct
Theisgomockmarker field is added by GoMock to prevent collisions. Auto-generated file—skipping detailed review.tests/main_test.go (1)
42-44: Path now anchored to the test file – verify it matches the generate location
filepath.Join(filepath.Dir(filename), "dist", "operator", …)correctly resolves relative to the test file, but it only works ifgo generate ./...is executed from the same directory beforego test. CI workflows that rungo test ./...without a prior generate step will still break.Please ensure your CI/Make/Just recipe invokes
go generate ./tests/...(or a globaljust generate) before running the suite.Earthfile (3)
10-11:COPY --dirwith multiple source paths may not do what you expect
Earthly’sCOPY --dirflag is documented to accept one directory tree to copy. Supplying multiple sources (cmd internal pkg) could silently ignore some paths or flatten the structure, depending on the Earthly version.If you need three directories preserved, emit three COPYs or create a single parent dir:
- COPY --dir cmd internal pkg . + COPY --dir cmd . + COPY --dir internal . + COPY --dir pkg .or
COPY --dir . .after pruning unrelated files.
21-28: Target rename to+buildlooks good – downstream references updated
The rename streamlines the pipeline, and subsequent references (BUILD --pass-args +build) have been adjusted accordingly. No further action required.
44-45: Confirm WAIT/BUILD indentation still passes syntax validation
Earthly is indentation-sensitive. Double-check that the innerBUILD --pass-args +build --tag=$tagline is indented exactly one tab underWAIT; inconsistent spacing will cause a parse error at build time.Justfile (6)
1-4: Well structured configuration setupThe Justfile is properly configured with
dotenv-loadenabled and a helpful default task that lists all available commands.
6-8: Good task organizationThe alias and task grouping provides convenient shortcuts and logical organization of tasks.
13-18: Excellent concurrent dependency managementThe tidy task efficiently runs
go mod tidyin parallel for both the root and tests directory with proper error handling viaset -euo pipefail.
20-25: Well-structured linting setupThe lint task properly depends on prerequisite tasks and runs linting concurrently with appropriate timeout and build tags.
30-35: Well-implemented integration test environment setupThe integration tests task correctly sets up the Kubernetes test environment with a specific version of envtest and runs tests in parallel.
46-50: Kubernetes connection and cleanup tasks work well togetherThe connect-dev and uninstall tasks are well-structured with proper dependency relationships and error handling.
internal/grpcclient/connection_generated.go (4)
1-8: Correctly auto-generated mock fileThis is an auto-generated mock file created by MockGen, as indicated by the header. The generation command is correctly specified.
9-18: Proper package and import setupThe package declaration and imports are correctly organized for the generated mocks.
20-86: MockConnection implementation follows best practicesThe MockConnection implementation correctly implements the Connection interface with proper helper methods, recorder setup, and type assertions.
87-152: MockConnectionAdapter implementation follows best practicesThe MockConnectionAdapter implementation correctly implements the ConnectionAdapter interface with proper context handling in each method.
| //go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcclient . Connection | ||
| type Connection interface { | ||
| CloseSend() error | ||
| Send(*generated.Message) error | ||
| Recv() (*generated.Order, error) | ||
| } | ||
|
|
||
| //go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcserver . ConnectionAdapter | ||
| //go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcclient . ConnectionAdapter |
There was a problem hiding this comment.
Duplicate mock generation destination
Both go:generate directives write to the same file (connection_generated.go), so the second will overwrite the first. Merge interfaces into one invocation or target separate destinations. For example:
-//go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcclient . Connection
-//go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcclient . ConnectionAdapter
+//go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcclient . Connection,ConnectionAdapter📝 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.
| //go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcclient . Connection | |
| type Connection interface { | |
| CloseSend() error | |
| Send(*generated.Message) error | |
| Recv() (*generated.Order, error) | |
| } | |
| //go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcserver . ConnectionAdapter | |
| //go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcclient . ConnectionAdapter | |
| //go:generate mockgen -source=connection_adapter.go -destination=connection_generated.go -package grpcclient . Connection,ConnectionAdapter | |
| type Connection interface { | |
| CloseSend() error | |
| Send(*generated.Message) error | |
| Recv() (*generated.Order, error) | |
| } | |
| // …rest of file (e.g. ConnectionAdapter interface/implementation)… |
🤖 Prompt for AI Agents (early access)
In internal/grpcclient/connection_adapter.go around lines 17 to 24, the two go:generate directives for mockgen both output to the same file connection_generated.go, causing the second to overwrite the first. To fix this, either combine the interfaces into a single mockgen command or change the destination file names so each interface's mock is generated into a separate file, preventing overwrites.
Dav-14
force-pushed
the
feat/use-just-nix
branch
2 times, most recently
from
4d5b457 to
9d6d8f4
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Workflow Tests Job Issues
The Tests job in the workflow contains three issues:
- YAML Indentation Error: Its steps are incorrectly indented with 4 spaces instead of 6, which will cause a YAML parsing error and workflow failure.
- Misleading Step Name: The step named "Pre-commit" is mislabeled, as it executes
just tests-all(tests) instead of pre-commit checks. - Missing Job Dependency: The job's dependency on the
Dirtyjob was removed, allowing tests to run concurrently with the dirty check and potentially on uncommitted changes.
.github/workflows/main.yml#L52-L66
agent/.github/workflows/main.yml
Lines 52 to 66 in 86e3f08
Bug: Missing Secrets in GitHub Actions Jobs
The GoReleaser and Release GitHub Actions jobs are missing required environment variables. SPEAKEASY_API_KEY and FURY_TOKEN were removed from both jobs, and GORELEASER_KEY was removed from the Release job in releases.yml. These secrets may still be required by the underlying goreleaser process.
.github/workflows/main.yml#L88-L91
agent/.github/workflows/main.yml
Lines 88 to 91 in 86e3f08
.github/workflows/releases.yml#L26-L29
agent/.github/workflows/releases.yml
Lines 26 to 29 in 86e3f08
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Justfile(1 hunks)internal/membership_listener_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Justfile
🔇 Additional comments (1)
internal/membership_listener_test.go (1)
44-45: Path construction looks correct but verify the directory structure.The CRD path update correctly points to the cloned operator directory structure. The use of
filepath.Joinandfilepath.Dir(filename)ensures proper path construction across different operating systems.The implementation properly constructs the path to the CRD bases directory within the cloned operator repository.
| //go:generate rm -rf ./dist/operator | ||
| //go:generate git clone --depth 1 --branch main https://github.com/formancehq/operator.git ./dist/operator |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider the implications of introducing network dependency and potential race conditions.
The go:generate directives introduce a network dependency for tests and could cause issues in CI/CD environments or when running tests concurrently. The rm -rf ./dist/operator command could also be risky if executed from an unexpected working directory.
Consider these potential improvements:
- Add error handling for git clone failures
- Use a more specific cleanup pattern to avoid accidental deletions
- Consider caching the cloned repository to reduce build time
- Document the network requirement for running tests
Run the following script to verify how this affects test execution:
🏁 Script executed:
#!/bin/bash
# Description: Check if tests can run without network access and verify concurrent execution safety
# Test 1: Check if any other tests also use similar go:generate directives
rg -A 3 -B 1 "go:generate.*git clone"
# Test 2: Check if there are any cleanup or setup scripts that might conflict
fd -t f -e go -x rg -l "dist/operator" {}
# Test 3: Check if there are any CI configuration files that might need updates
fd -t f -e yml -e yaml -x rg -l "test" {} | head -5Length of output: 985
Review go:generate directives for operator clone in tests
The new //go:generate lines in both
internal/membership_listener_test.go(lines 33–34)tests/main_test.go
introduce a network dependency at test-time and use an unscoped rm -rf ./dist/operator, which can be unsafe if the working directory changes.
Recommended fixes:
- Wrap the
git clonein a small shell script or Makefile target that checks for errors and prints a clear failure message. - Scope cleanup to only the expected path (e.g. use a temp directory or verify that
./dist/operatorexists before removing). - Cache or vendor the cloned repo (for example, clone once in CI and mount it, or use
go:embed/a git submodule) to avoid repeated network fetches. - Document the requirement for network access (and any authentication) in your CONTRIBUTING.md or test README.
- Update CI workflows to prepare
./dist/operatorahead of test runs, so tests don’t depend on live cloning.
🤖 Prompt for AI Agents
In internal/membership_listener_test.go at lines 33-34, the go:generate
directives currently perform an unsafe unscoped rm -rf on ./dist/operator and
clone the operator repo directly during test generation, causing network
dependency and potential data loss. To fix this, replace the direct rm and git
clone commands with a shell script or Makefile target that first checks if
./dist/operator exists before removing it, handles errors gracefully with clear
messages, and caches or vendors the cloned repo to avoid repeated network
fetches. Also, document the network access requirement in CONTRIBUTING.md or
test README and update CI workflows to prepare ./dist/operator before tests run,
eliminating live cloning during test generation.
No description provided.