-
Notifications
You must be signed in to change notification settings - Fork 11
chore: envs setup #335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: envs setup #335
Conversation
📝 WalkthroughWalkthroughAdds three environment files (dev, dev.2, mutinynet) and refactors the Makefile to add a reusable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(2 hunks)envs/dev.2.env(1 hunks)envs/dev.env(1 hunks)envs/mutinynet.env(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
⏰ 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: unit tests
- GitHub Check: integration tests
🔇 Additional comments (6)
envs/mutinynet.env (1)
1-8: LGTM! Mutinynet configuration looks good.The environment variables are appropriately configured for the mutinynet environment with secure endpoints (HTTPS/WSS) and reasonable defaults for polling intervals and timeouts.
envs/dev.2.env (1)
1-10: LGTM! Secondary dev environment properly configured.The configuration correctly uses a separate datadir (
./datadir-2) and explicit ports (7008, 7009) to enable running a second development instance alongside the primary one without conflicts.Makefile (3)
1-1: PHONY targets updated correctly.The new targets
run-mutinynetandrun-2are properly declared as phony targets.Note: The static analysis tool flags a missing "all" target, but this is an optional convention, not a requirement.
53-55: Run target refactored correctly.The migration from inline environment exports to the
setup_envmacro improves consistency and maintainability. Ensure the macro implementation issues are addressed.
57-63: New targets configured correctly.Both
run-2andrun-mutinynettargets follow the same pattern as the refactoredruntarget and correctly reference their respective environment files. The newrun-mutinynettarget provides a convenient way to test against the mutinynet environment.envs/dev.env (1)
1-8: The dev.env configuration is correct and intentional—no port conflicts occur.The application defines default ports in internal/config/config.go: defaultGRPCPort = 7000 and defaultHTTPPort = 7001. The dev.env file intentionally omits FULMINE_GRPC_PORT and FULMINE_HTTP_PORT to use these defaults. Since dev.2.env explicitly sets ports 7008 and 7009, all four ports (7000, 7001, 7008, 7009) are distinct and there is no conflict when both instances run simultaneously.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
57-63: Add documentation comments for consistency.The new targets
run-2andrun-mutinynetare missing##documentation comments, while most other targets in the Makefile include them (e.g., lines 17, 22, 27, etc.). Adding these comments improves discoverability viamake help.📝 Suggested documentation additions
+## run-2: run in dev mode with alternative configuration run-2: clean build-static-assets $(call setup_env, envs/dev.2.env) go run ./cmd/fulmine +## run-mutinynet: run in mutinynet environment run-mutinynet: clean build-static-assets $(call setup_env, envs/mutinynet.env) go run ./cmd/fulmine
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
⏰ 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: unit tests
- GitHub Check: integration tests
🔇 Additional comments (2)
Makefile (2)
1-1: LGTM: PHONY targets correctly declared.The new targets
run-mutinynetandrun-2are properly added to the.PHONYdeclaration, following Makefile conventions for non-file targets.
54-54: Consistent use of setup_env macro.The
runtarget correctly uses the newsetup_envmacro to load environment variables fromenvs/dev.env, maintaining consistency with the other run targets.
@Kukks @altafan please review
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.