feat: Use Nix & Justfile #619
Conversation
- Add ginkgo to the list of dependencies in flake.nix - Remove unnecessary CACHE directives in Earthfile - Simplify 'compile' section in Earthfile - Simplify 'tests' section in Earthfile - Simplify 'lint' section in Earthfile - Simplify 'pre-commit' section in Earthfile - Simplify 'pre-commit-nix' section in Earthfile - Simplify 'tidy' section in Earthfile - Simplify 'export-docs-events' section in Earthfile feat/nix
Reworked config to use 'nix develop' with 'earthly' command. Added 'pre-commit-nix' hook. Removed unnecessary flags and secrets. (feat/nix)
WalkthroughThis pull request introduces changes to the project's development infrastructure, focusing on environment configuration and build processes. The modifications include adding a Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Nitpick comments (2)
flake.nix (1)
42-42: Consider upgrading to JDK 17 LTSThe configuration uses JDK 11, which is an older LTS version. Consider upgrading to JDK 17 LTS for better performance, security, and longer support.
- jdk11 + jdk17Earthfile (1)
151-165: Consider simplifying the release mode logicThe current implementation could be simplified by:
- Using a more direct condition for docker login
- Consolidating the buildArgs logic
LOCALLY ARG mode=local - # TODO: Move to function in earthly repostiory - LET buildArgs = --clean - IF [ "$mode" = "local" ] - SET buildArgs = --nightly --skip=publish --clean - ELSE IF [ "$mode" = "ci" ] - SET buildArgs = --nightly --clean - END - IF [ "$mode" != "local" ] + LET buildArgs = "--nightly --clean" + IF [ "$mode" = "local" ] + SET buildArgs = "$buildArgs --skip=publish" + ELSE WITH DOCKER RUN --secret GITHUB_TOKEN echo $GITHUB_TOKEN | docker login ghcr.io -u NumaryBot --password-stdin END END
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/main.ymlis excluded by!**/*.ymlflake.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
.gitignore(1 hunks)Earthfile(7 hunks)flake.nix(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (3)
flake.nix (2)
39-39: Verify goreleaser-pro license and availability
The configuration uses goreleaser-pro from NUR. Please ensure:
- The team has the necessary licenses for goreleaser-pro
- The CI environment has access to the required license key
5-5: Consider pinning the nixpkgs version for reproducible builds
Using a wildcard version (0.1.*) could lead to inconsistent builds across different environments. Consider pinning to a specific version or commit hash.
Earthfile (1)
205-207: LGTM!
The change to run docs events generation locally is straightforward and correct.
Reworked config to use 'nix develop' with 'earthly' command. Added 'pre-commit-nix' hook. Removed unnecessary flags and secrets. (feat/nix)
Reworked config to use 'nix develop' with 'earthly' command. Added 'pre-commit-nix' hook. Removed unnecessary flags and secrets. (feat/nix)
Reworked config to use 'nix develop' with 'earthly' command. Added 'pre-commit-nix' hook. Removed unnecessary flags and secrets. (feat/nix)
Reworked config to use 'nix develop' with 'earthly' command. Added 'pre-commit-nix' hook. Removed unnecessary flags and secrets. (feat/nix)
Reworked config to use 'nix develop' with 'earthly' command. Added 'pre-commit-nix' hook. Removed unnecessary flags and secrets. (feat/nix)
Reworked config to use 'nix develop' with 'earthly' command. Added 'pre-commit-nix' hook. Removed unnecessary flags and secrets. (feat/nix)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #619 +/- ##
=======================================
Coverage ? 64.96%
=======================================
Files ? 8
Lines ? 314
Branches ? 0
=======================================
Hits ? 204
Misses ? 97
Partials ? 13 ☔ View full report in Codecov by Sentry. |
Reworked config to use 'nix develop' with 'earthly' command. Added 'pre-commit-nix' hook. Removed unnecessary flags and secrets. (feat/nix)
Added a new .envrc file with configuration for direnv and updated .gitignore to include .envrc while still ignoring .env files. This ensures proper local environment setup using direnv without accidentally committing sensitive .env files.
This change ensures all mock test files include the `isgomock` struct, standardizing the structure of generated mocks. This addition aligns with gomock's conventions and facilitates internal consistency across mock definitions.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.envrc(1 hunks).gitignore(1 hunks)Earthfile(8 hunks)internal/api/bulking/mocks_ledger_controller_test.go(2 hunks)internal/api/common/mocks_ledger_controller_test.go(2 hunks)internal/api/common/mocks_system_controller_test.go(2 hunks)internal/api/v1/mocks_ledger_controller_test.go(2 hunks)internal/api/v1/mocks_system_controller_test.go(2 hunks)internal/api/v2/mocks_ledger_controller_test.go(2 hunks)internal/api/v2/mocks_system_controller_test.go(2 hunks)internal/controller/ledger/controller_generated_test.go(2 hunks)internal/controller/ledger/controller_with_too_many_client_handling_generated_test.go(2 hunks)internal/controller/ledger/listener_generated_test.go(2 hunks)internal/controller/ledger/numscript_parser_generated_test.go(2 hunks)internal/controller/ledger/numscript_runtime_generated_test.go(2 hunks)internal/controller/ledger/store_generated_test.go(2 hunks)internal/storage/driver/buckets_generated_test.go(3 hunks)internal/storage/driver/ledger_generated_test.go(2 hunks)internal/storage/driver/system_generated_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (11)
- internal/storage/driver/ledger_generated_test.go
- internal/controller/ledger/numscript_parser_generated_test.go
- internal/storage/driver/system_generated_test.go
- internal/controller/ledger/controller_generated_test.go
- internal/api/common/mocks_ledger_controller_test.go
- internal/api/v2/mocks_system_controller_test.go
- internal/controller/ledger/store_generated_test.go
- internal/api/v1/mocks_system_controller_test.go
- internal/controller/ledger/controller_with_too_many_client_handling_generated_test.go
- internal/controller/ledger/listener_generated_test.go
- internal/storage/driver/buckets_generated_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- Earthfile
🔇 Additional comments (5)
internal/controller/ledger/numscript_runtime_generated_test.go (1)
21-21: LGTM! Standard gomock upgrade change.
The addition of the isgomock field is a standard change from upgrading to a newer version of go.uber.org/mock. This is a non-functional change that helps gomock with internal type assertions.
internal/api/common/mocks_system_controller_test.go (1)
24-24: LGTM! Consistent mock upgrade change.
The addition of the isgomock field matches the expected changes from the gomock upgrade across all mock files.
internal/api/v1/mocks_ledger_controller_test.go (2)
6-7: LGTM: Mockgen command is properly configured.
The mockgen command is correctly configured with appropriate flags for generating the mock implementation.
26-26: Verify mockgen version compatibility.
The addition of the isgomock field suggests a different version of mockgen is being used. This is likely due to the Nix environment setup.
✅ Verification successful
Let me try a different approach to gather the necessary information.
Let me check other mock files to verify if this is a consistent pattern with the current mockgen version.
The isgomock field is consistent across mock implementations and is part of the current mockgen version.
The verification shows that:
- The
isgomock struct{}field is present in other mock files (confirmed in v2 implementation) - All mock files are generated using the same mockgen configuration with consistent flags
- This is a standard field added by the current version of mockgen and not a version compatibility issue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockgen version and configuration across the project
# Find all mock files and check for isgomock field consistency
echo "Checking mock files for isgomock field consistency..."
rg -l "isgomock struct{}" --type go
# Look for mockgen version specification in project files
echo "Looking for mockgen version specifications..."
rg -l "mockgen" --type nix
Length of output: 408
Script:
#!/bin/bash
# Check mockgen version in flake.nix
echo "Checking mockgen version in flake.nix..."
cat flake.nix | grep -A 5 mockgen
# Find all mock files
echo -e "\nListing all mock files..."
fd -t f "mock.*\.go$"
# Check the content of the specific mock file
echo -e "\nChecking the specific mock file..."
cat internal/api/v1/mocks_ledger_controller_test.go
Length of output: 17052
Script:
#!/bin/bash
# Check the content of another mock file for comparison
echo "Checking another mock file..."
cat internal/api/v2/mocks_ledger_controller_test.go | grep -A 5 "type LedgerController struct"
# Check mockgen command used in other files
echo -e "\nChecking mockgen commands in other files..."
rg "mockgen.*-source.*-destination" --type go
Length of output: 8710
internal/api/bulking/mocks_ledger_controller_test.go (1)
26-26: LGTM: Consistent mock implementation.
The changes are consistent with other mock files, maintaining uniformity across the codebase.
| @@ -0,0 +1,2 @@ | |||
| use flake --impure | |||
There was a problem hiding this comment.
💡 Codebase verification
Add documentation and .env.example for development setup
The verification revealed:
- The CI workflows use
--impureflag in a controlled environment with proper secrets management - No documentation exists about Nix setup or required environment variables
- No
.env.exampletemplate is present, only.envrcexists
Please add:
- Documentation in README.md about the Nix-based development setup and the implications of using
--impure - An
.env.examplefile listing all required environment variables for development
🔗 Analysis chain
Consider security implications of using --impure flag
The --impure flag allows the Nix environment to access network and mutable state, which could potentially introduce non-deterministic builds or security risks. While this is often necessary for development environments, please ensure:
- This is documented in the project's development setup guide
- CI/CD pipelines use pure evaluation for reproducible builds
Let's verify if there's documentation about this:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for development setup documentation
rg -i "nix|flake|direnv" README.md || echo "No documentation found about Nix setup"
# Check if CI/CD uses pure evaluation
rg --type yaml "use flake" .github/
Length of output: 161
Script:
#!/bin/bash
# Let's check for any Nix-related documentation in other common locations
fd -t f -e md . | xargs rg -i "nix|flake|direnv" || echo "No Nix docs found"
# Check if flake.nix exists and its contents
if [ -f "flake.nix" ]; then
echo "=== flake.nix contents ==="
cat flake.nix
else
echo "flake.nix not found"
fi
# Check for CI workflow files
fd -t f . .github/workflows/ || echo "No workflow files found"
Length of output: 1550
Script:
#!/bin/bash
# Let's check CI workflows for Nix-related configuration
echo "=== Checking CI workflows for Nix usage ==="
cat .github/workflows/main.yml
cat .github/workflows/releases.yml
cat .github/workflows/benchmark.yml
# Check for any .env related files
echo -e "\n=== Checking for .env related files ==="
fd -H "\.env" || echo "No .env files found"
Length of output: 8036
This change replaces Earthly with Just by removing Earthfiles and introducing a Justfile. Benefits include streamlining the dependency management, simplifying workflow automation, and improving build process readability. Build and CI workflows (e.g., pre-commit, tests, linting) have been adapted to utilize Just commands, ensuring consistency across development processes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
flake.nix (1)
32-44: Consider adding essential development toolsThe development shell includes a good set of tools, but consider adding:
delvefor debugginggoplsfor IDE supportairfor live reload during developmentpackages = with pkgs; [ go gotools golangci-lint ginkgo yq jq pkgs.nur.repos.goreleaser.goreleaser-pro mockgen gomarkdoc jdk11 just + delve + gopls + air ];Justfile (2)
6-9: Consider adding validation to pre-commitThe pre-commit chain looks good but consider adding:
teststo ensure changes don't break existing functionality- A command to verify the Nix environment is properly set up
-pre-commit: tidy lint generate export-docs-events earthly +pre-commit: verify-env tidy lint generate export-docs-events tests earthly +verify-env: + @command -v nix >/dev/null 2>&1 || { echo "Nix is not installed. Please install Nix first."; exit 1; } + @nix flake check
28-39: Consider improving test coverage reportingThe test configuration is good but could be enhanced:
- Add minimum coverage threshold
- Generate HTML coverage report for better visualization
tests: @go test -race -covermode=atomic \ -coverpkg=github.com/formancehq/ledger/internal/... \ -coverpkg=github.com/formancehq/ledger/pkg/events/... \ -coverpkg=github.com/formancehq/ledger/pkg/accounts/... \ -coverpkg=github.com/formancehq/ledger/pkg/assets/... \ -coverpkg=github.com/formancehq/ledger/cmd/... \ -coverprofile coverage.txt \ -tags it \ ./... @cat coverage.txt | grep -v debug.go | grep -v "/machine/" > coverage2.txt @mv coverage2.txt coverage.txt + @go tool cover -html=coverage.txt -o coverage.html + @go tool cover -func=coverage.txt | grep total | awk '{print $3}' | sed 's/%//' | awk '{if ($1 < 80) exit 1}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/main.ymlis excluded by!**/*.yml
📒 Files selected for processing (5)
Earthfile(1 hunks)Justfile(1 hunks)deployments/pulumi/Earthfile(0 hunks)flake.nix(1 hunks)tools/generator/Earthfile(2 hunks)
💤 Files with no reviewable changes (1)
- deployments/pulumi/Earthfile
🚧 Files skipped from review as they are similar to previous changes (1)
- Earthfile
🔇 Additional comments (5)
flake.nix (2)
14-14: Go 1.23 is not yet released
The specified Go version (23) corresponds to Go 1.23, which is not yet released. The current stable version is Go 1.21.5.
16-24: LGTM: Comprehensive system architecture support
The system configuration correctly handles multiple architectures (x86_64 and aarch64) for both Linux and Darwin, with proper overlay configuration and unfree package support.
Justfile (1)
1-4: LGTM: Environment configuration
The dotenv-load setting and default task listing provide good developer experience.
tools/generator/Earthfile (2)
14-19: LGTM: Simplified COPY commands
The path simplification for COPY commands improves readability while maintaining functionality.
41-41: Verify image repository configuration
Ensure the repository configuration aligns with the new Nix-based development environment.
✅ Verification successful
The verification results show consistent usage of ghcr.io across different files in the repository, including Earthfiles, Dockerfiles, and deployment configurations. The repository configuration in tools/generator/Earthfile matches the pattern used in other files and is properly aligned with the build system. There's no indication of any Nix-specific repository configuration that would conflict with the current setup.
Repository configuration is correctly aligned
The REPOSITORY argument defaulting to ghcr.io in the generator Earthfile is consistent with the project-wide container registry configuration, and there are no conflicts with development environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the repository configuration is consistent across files
rg -g '!.git' -l "ghcr.io" | while read -r file; do
echo "Found repository reference in: $file"
rg "ghcr.io" "$file" -C 2
done
Length of output: 1621
Replaced complex earthly-based release command with a `just` target in the CI workflow for better maintainability. Added new `release` commands to the Justfile for local, CI, and standard releases. This streamlines the release process and enhances clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Justfile (4)
6-9: Consider preserving Earthly output for debuggingThe
@prefix in the earthly command suppresses all output. This might make it difficult to debug issues when they occur.- @earthly --no-output +pre-commit + earthly --no-output +pre-commit
17-20: Consider adding error handling and path validationThe
tidytask assumes the directories exist and the commands succeed. Consider adding error checking and making the paths configurable.tidy: - @go mod tidy - @cd {{justfile_directory()}}/tools/generator && go mod tidy - @cd {{justfile_directory()}}/deployments/pulumi && go mod tidy + #!/usr/bin/env sh + set -e + go mod tidy + for dir in tools/generator deployments/pulumi; do + if [ -d "$dir" ]; then + cd "$dir" && go mod tidy && cd {{justfile_directory()}} + else + echo "Warning: Directory $dir not found" + fi + done
22-26: Consider making the documentation path configurableThe documentation export path is hardcoded. Consider making it configurable through an environment variable for flexibility.
export-docs-events: - @go run . docs events --write-dir docs/events + @go run . docs events --write-dir ${DOCS_DIR:-docs/events}
41-48: Consider adding release validation checksThe release tasks could benefit from pre-release validation checks to ensure all conditions are met before releasing.
release: + #!/usr/bin/env sh + if [ -n "$(git status --porcelain)" ]; then + echo "Error: Working directory is not clean" + exit 1 + fi + if [ -z "${GITHUB_TOKEN}" ]; then + echo "Error: GITHUB_TOKEN is not set" + exit 1 + fi @goreleaser release --clean
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/main.ymlis excluded by!**/*.yml
📒 Files selected for processing (2)
Earthfile(1 hunks)Justfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Earthfile
🔇 Additional comments (2)
Justfile (2)
1-4: Verify .env file handling
The dotenv-load directive is enabled, which is good for local development. However, let's ensure proper safeguards are in place.
✅ Verification successful
Environment variable handling is properly configured
The verification confirms that:
.envis correctly listed in.gitignore- No
.envfiles are currently committed to the repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify .env is properly gitignored and no .env files are committed
# Check if .env is in .gitignore
rg "^\.env$" .gitignore
# Verify no .env files are committed
git ls-files | rg "\.env$"
Length of output: 59
11-15: Review auto-fix behavior in linting
The --fix flag automatically modifies code. This could be risky in CI environments or when running pre-commit hooks, as it might introduce unexpected changes.
Consider:
- Making auto-fix optional
- Running lint check first, then fix separately
- Verifying if the 5m timeout is sufficient
| tests: | ||
| @go test -race -covermode=atomic \ | ||
| -coverpkg=github.com/formancehq/ledger/internal/... \ | ||
| -coverpkg=github.com/formancehq/ledger/pkg/events/... \ | ||
| -coverpkg=github.com/formancehq/ledger/pkg/accounts/... \ | ||
| -coverpkg=github.com/formancehq/ledger/pkg/assets/... \ | ||
| -coverpkg=github.com/formancehq/ledger/cmd/... \ | ||
| -coverprofile coverage.txt \ | ||
| -tags it \ | ||
| ./... | ||
| @cat coverage.txt | grep -v debug.go | grep -v "/machine/" > coverage2.txt | ||
| @mv coverage2.txt coverage.txt |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve coverage file handling
The current coverage file manipulation using grep and mv could fail silently. Consider using a more robust approach.
tests:
@go test -race -covermode=atomic \
-coverpkg=github.com/formancehq/ledger/internal/... \
-coverpkg=github.com/formancehq/ledger/pkg/events/... \
-coverpkg=github.com/formancehq/ledger/pkg/accounts/... \
-coverpkg=github.com/formancehq/ledger/pkg/assets/... \
-coverpkg=github.com/formancehq/ledger/cmd/... \
-coverprofile coverage.txt \
-tags it \
./...
- @cat coverage.txt | grep -v debug.go | grep -v "/machine/" > coverage2.txt
- @mv coverage2.txt coverage.txt
+ @if [ -f coverage.txt ]; then \
+ grep -v 'debug\.go\|/machine/' coverage.txt > coverage.tmp && \
+ mv coverage.tmp coverage.txt || \
+ echo "Failed to process coverage file"; \
+ fi📝 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.
| tests: | |
| @go test -race -covermode=atomic \ | |
| -coverpkg=github.com/formancehq/ledger/internal/... \ | |
| -coverpkg=github.com/formancehq/ledger/pkg/events/... \ | |
| -coverpkg=github.com/formancehq/ledger/pkg/accounts/... \ | |
| -coverpkg=github.com/formancehq/ledger/pkg/assets/... \ | |
| -coverpkg=github.com/formancehq/ledger/cmd/... \ | |
| -coverprofile coverage.txt \ | |
| -tags it \ | |
| ./... | |
| @cat coverage.txt | grep -v debug.go | grep -v "/machine/" > coverage2.txt | |
| @mv coverage2.txt coverage.txt | |
| tests: | |
| @go test -race -covermode=atomic \ | |
| -coverpkg=github.com/formancehq/ledger/internal/... \ | |
| -coverpkg=github.com/formancehq/ledger/pkg/events/... \ | |
| -coverpkg=github.com/formancehq/ledger/pkg/accounts/... \ | |
| -coverpkg=github.com/formancehq/ledger/pkg/assets/... \ | |
| -coverpkg=github.com/formancehq/ledger/cmd/... \ | |
| -coverprofile coverage.txt \ | |
| -tags it \ | |
| ./... | |
| @if [ -f coverage.txt ]; then \ | |
| grep -v 'debug\.go\|/machine/' coverage.txt > coverage.tmp && \ | |
| mv coverage.tmp coverage.txt || \ | |
| echo "Failed to process coverage file"; \ | |
| fi |
| ARG mode=local | ||
| COPY --dir . /src | ||
| DO core+GORELEASER --mode=$mode | ||
|
|
||
| generate-client: |
There was a problem hiding this comment.
Maybe this target can be extracted in the Justfile too?
- Removed unused 'group' annotation in Justfile to enhance clarity. - Consolidated COPY commands in Earthfile for better maintainability. - Updated 'compile' stage in Earthfile to use 'sources' for consistency. These changes reduce redundancy, improve readability, and ensure less complexity in maintenance.
Reordered tasks in the pre-commit command to ensure `generate` and `earthly` execute before `tidy`, `lint`, and `export-docs-events`. This improves maintainability and ensures prerequisites are met.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Justfile (3)
11-14: Consider making lint timeout configurableThe 5-minute timeout is hardcoded. Consider making it configurable through an environment variable for flexibility in different environments.
lint: - @golangci-lint run --fix --build-tags it --timeout 5m - @cd {{justfile_directory()}}/tools/generator && golangci-lint run --fix --build-tags it --timeout 5m - @cd {{justfile_directory()}}/deployments/pulumi && golangci-lint run --fix --build-tags it --timeout 5m + @golangci-lint run --fix --build-tags it --timeout ${LINT_TIMEOUT:-5m} + @cd {{justfile_directory()}}/tools/generator && golangci-lint run --fix --build-tags it --timeout ${LINT_TIMEOUT:-5m} + @cd {{justfile_directory()}}/deployments/pulumi && golangci-lint run --fix --build-tags it --timeout ${LINT_TIMEOUT:-5m}
24-25: Consider making docs directory configurableThe documentation output directory is hardcoded. Consider making it configurable through an environment variable.
export-docs-events: - @go run . docs events --write-dir docs/events + @go run . docs events --write-dir ${DOCS_DIR:-docs/events}
40-47: Add release command validation and documentationConsider:
- Adding environment validation before releases
- Adding comments to clarify the differences between release commands
+# Local release for testing, skips publishing release-local: + @[ -n "$(git status --porcelain)" ] && echo "Working directory not clean" && exit 1 @goreleaser release --nightly --skip=publish --clean +# CI release with nightly tag release-ci: + @[ -n "${CI}" ] || (echo "Not in CI environment" && exit 1) @goreleaser release --nightly --clean +# Production release release: + @[ -n "$(git tag --points-at HEAD)" ] || (echo "No tag found" && exit 1) @goreleaser release --clean
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Justfile(1 hunks)tools/generator/Earthfile(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/generator/Earthfile
🔇 Additional comments (2)
Justfile (2)
1-4: LGTM! Good configuration setup
The dotenv-load setting and default command listing provide good developer experience.
27-38: Improve coverage file handling
The current coverage file manipulation using grep and mv could fail silently. Consider using a more robust approach.
Justfile
Outdated
| pre-commit: generate earthly tidy lint export-docs-events | ||
|
|
||
| earthly: | ||
| @earthly --no-output +pre-commit |
There was a problem hiding this comment.
Fix potential infinite recursion in pre-commit workflow
There's a circular dependency:
pre-commitincludesearthlyin its sequenceearthlycommand runs+pre-committarget
This could lead to infinite recursion. Consider either:
- Removing
earthlyfrom thepre-commitsequence, or - Having
earthlyrun a different target that doesn't includepre-commit
| tidy: | ||
| @go mod tidy | ||
| @cd {{justfile_directory()}}/tools/generator && go mod tidy | ||
| @cd {{justfile_directory()}}/deployments/pulumi && go mod tidy |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for directory changes
The cd commands should handle errors and ensure we return to the original directory.
tidy:
@go mod tidy
- @cd {{justfile_directory()}}/tools/generator && go mod tidy
- @cd {{justfile_directory()}}/deployments/pulumi && go mod tidy
+ @cd {{justfile_directory()}}/tools/generator && go mod tidy || exit 1
+ @cd {{justfile_directory()}}/deployments/pulumi && go mod tidy || exit 1📝 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.
| tidy: | |
| @go mod tidy | |
| @cd {{justfile_directory()}}/tools/generator && go mod tidy | |
| @cd {{justfile_directory()}}/deployments/pulumi && go mod tidy | |
| tidy: | |
| @go mod tidy | |
| @cd {{justfile_directory()}}/tools/generator && go mod tidy || exit 1 | |
| @cd {{justfile_directory()}}/deployments/pulumi && go mod tidy || exit 1 |
Replaced `earthly` with `nix` and `just` in the release workflow. This improves build consistency and leverages flakes for reproducibility.
* feat: improve queries performance and refactor storage read part * fix: post commit effective volumes rendering * fix: from review
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
flake.nix (1)
14-14:⚠️ Potential issueGo 1.23 is not yet released
The specified Go version (23) corresponds to Go 1.23, which is not yet released. The current stable version is Go 1.21.5.
Apply this fix:
- goVersion = 23; # Change this to update the whole stack + goVersion = 21; # Change this to update the whole stackJustfile (3)
9-10:⚠️ Potential issueFix potential infinite recursion in pre-commit workflow
The
earthlycommand runs+pre-committarget which could create an infinite loop.Consider either:
- Removing
earthlyfrom thepre-commitsequence, or- Having
earthlyrun a different target that doesn't includepre-commit
31-42: 🛠️ Refactor suggestionImprove coverage file handling
The current coverage file manipulation using
grepandmvcould fail silently.Apply this fix:
tests: @go test -race -covermode=atomic \ -coverpkg=github.com/formancehq/ledger/internal/... \ -coverpkg=github.com/formancehq/ledger/pkg/events/... \ -coverpkg=github.com/formancehq/ledger/pkg/accounts/... \ -coverpkg=github.com/formancehq/ledger/pkg/assets/... \ -coverpkg=github.com/formancehq/ledger/cmd/... \ -coverprofile coverage.txt \ -tags it \ ./... - @cat coverage.txt | grep -v debug.go | grep -v "/machine/" > coverage2.txt - @mv coverage2.txt coverage.txt + @if [ -f coverage.txt ]; then \ + grep -v 'debug\.go\|/machine/' coverage.txt > coverage.tmp && \ + mv coverage.tmp coverage.txt || \ + echo "Failed to process coverage file"; \ + fi
17-20: 🛠️ Refactor suggestionAdd error handling for directory changes
The
cdcommands should handle errors and ensure we return to the original directory.Apply this fix:
tidy: @go mod tidy - @cd {{justfile_directory()}}/tools/generator && go mod tidy - @cd {{justfile_directory()}}/deployments/pulumi && go mod tidy + @cd {{justfile_directory()}}/tools/generator && go mod tidy || exit 1 + @cd {{justfile_directory()}}/deployments/pulumi && go mod tidy || exit 1
🧹 Nitpick comments (2)
flake.nix (1)
32-44: Consider adding essential Go development toolsThe development shell includes a good set of tools, but could benefit from additional essential Go development tools.
Consider adding these commonly used tools:
packages = with pkgs; [ go gotools golangci-lint ginkgo yq-go jq pkgs.nur.repos.goreleaser.goreleaser-pro mockgen gomarkdoc jdk11 just + delve # Go debugger + gopls # Go language server + go-tools # Additional analysis tools ];Earthfile (1)
78-78: Simplify client generation path handlingThe client generation path could be simplified by using relative paths.
Apply this change:
- COPY openapi.yaml openapi.yaml + COPY (+openapi/openapi.yaml) ./
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
openapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (5)
.gitignore(1 hunks)Earthfile(2 hunks)Justfile(1 hunks)flake.nix(1 hunks)internal/controller/ledger/store_generated_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- internal/controller/ledger/store_generated_test.go
🔇 Additional comments (1)
Earthfile (1)
24-24: Verify local compilation environment requirements
The switch to LOCALLY directive means the compilation will happen on the developer's machine instead of in a controlled Docker environment.
Run this script to check if all required tools are available in the Nix environment:
✅ Verification successful
Based on the examination of the codebase, I can now provide a final response:
The LOCALLY directive is safe as build tools are managed through flake.nix
The project uses Nix for development environment management through flake.nix, which ensures consistent tooling across developers' machines. The local compilation is intentional and properly supported:
- The repository contains flake.nix for Nix-based environment management
- Required Go tools are defined and managed through the Nix configuration
- The project documentation mentions using Earthly with local development setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all tools required for local compilation are available in the Nix environment
# Check if we're in a Nix shell
if ! command -v nix-shell >/dev/null 2>&1; then
echo "Not in a Nix shell environment"
exit 1
fi
# Check for required Go tools
tools=("go" "golangci-lint" "mockgen" "goreleaser")
for tool in "${tools[@]}"; do
if ! command -v "$tool" >/dev/null 2>&1; then
echo "Missing required tool: $tool"
exit 1
fi
done
Length of output: 102
Script:
#!/bin/bash
# Let's check if there's a shell.nix or flake.nix file that defines the development environment
fd "shell.nix|flake.nix" --type f
# Also check if there are any other files that might specify the build environment
fd "default.nix" --type f
# Let's also look for any documentation about development setup
fd "README.md|CONTRIBUTING.md|DEVELOPMENT.md" --type f --exec cat {} \; | grep -i "nix\|development\|setup\|prerequisites"
Length of output: 795
Script:
#!/bin/bash
# Let's examine the Earthfile to understand the build setup and requirements
cat Earthfile
# Also check if there are any Go-specific build requirements
fd "go.mod|go.sum" --type f --exec cat {} \;
# Look for any Docker-related files that might be relevant
fd "Dockerfile|.dockerignore" --type f --exec cat {} \;
Length of output: 112085
No description provided.