fix: correct port binding for non-default ports#112
fix: correct port binding for non-default ports#112anisaoshafi wants to merge 4 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdded an integration test that starts the service with a non-default port via a generated TOML config and mock license server. The docker runtime was changed to bind a fixed container port ("4566/tcp") instead of deriving the container port from the configured host port. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (1)
internal/container/start.go (1)
326-332: Consider extracting the default port to a constant to avoid duplication.The string
"4566"is hardcoded here, but the same default is also defined ininternal/config/config.go:setDefaults(). If the default port changes in one place but not the other, non-default port detection will silently break.♻️ Suggested approach
Define a constant in the config package and reference it from both locations:
// In internal/config/config.go const DefaultPort = "4566" func setDefaults() { viper.SetDefault("containers", []map[string]any{ { "type": "aws", "tag": "latest", "port": DefaultPort, }, }) }Then in
needsGatewayListen:func needsGatewayListen(port string, env []string) bool { - return port != "4566" && !slices.ContainsFunc(env, func(e string) bool { + return port != config.DefaultPort && !slices.ContainsFunc(env, func(e string) bool { return strings.HasPrefix(e, "GATEWAY_LISTEN=") }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/container/start.go` around lines 326 - 332, Extract the hardcoded "4566" into a shared constant (e.g. DefaultPort string) in the config package (add const DefaultPort = "4566" in internal/config/config.go and use it inside setDefaults()), then replace the literal "4566" in needsGatewayListen with config.DefaultPort and add the config import in internal/container/start.go; ensure all references to the default port use config.DefaultPort so non-default detection remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/container/start.go`:
- Around line 326-332: Extract the hardcoded "4566" into a shared constant (e.g.
DefaultPort string) in the config package (add const DefaultPort = "4566" in
internal/config/config.go and use it inside setDefaults()), then replace the
literal "4566" in needsGatewayListen with config.DefaultPort and add the config
import in internal/container/start.go; ensure all references to the default port
use config.DefaultPort so non-default detection remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 08836855-fa8f-4c4d-8368-99ea054603a3
📒 Files selected for processing (2)
internal/container/start.gotest/integration/start_test.go
When configuring a non-default port (e.g. 4567), the Docker binding was 4567:4567 instead of 4567:4566, so the health check could not reach localstack and lstk would hang.
Fix port binding for non-default ports so the emulator actually becomes ready.