fix: correct port binding for non-default ports#112
Conversation
anisaoshafi
requested review from
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthroughIntroduces a container-level port mapping and plumbing: a new ContainerPort field and ContainerPort() lookup, propagates containerPort through container start logic and Docker bindings, and adds an integration test that starts the service with a non-default host port via a generated TOML config. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
anisaoshafi
changed the title
anisaoshafi
force-pushed
the
drg-611
branch
2 times, most recently
from
7c93e09 to
4a6b0e9
Compare
internal/runtime/docker.go
Outdated
| return nil | ||
| } | ||
|
|
||
| const emulatorContainerPort = nat.Port("4566/tcp") |
There was a problem hiding this comment.
thought: ultimately, when we support other emulators, we will to configure it per emulator and might want to move this value to runtime.ContainerConfig, but we can do it later!
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/config/containers.go (1)
34-36: Avoid introducing another mutable package-level map for port resolution.Line 34–36 adds new shared global state. Prefer resolving in
ContainerPort()viaswitch(or immutable config injected at construction) to keep test isolation tighter.♻️ Suggested refactor
-var emulatorContainerPorts = map[EmulatorType]string{ - EmulatorAWS: "4566/tcp", -} - func (c *ContainerConfig) ContainerPort() (string, error) { - port, ok := emulatorContainerPorts[c.Type] - if !ok { - return "", fmt.Errorf("%s emulator not supported yet by lstk", c.Type) - } - return port, nil + switch c.Type { + case EmulatorAWS: + return "4566/tcp", nil + default: + return "", fmt.Errorf("%s emulator not supported yet by lstk", c.Type) + } }As per coding guidelines: "Keep packages testable in isolation by avoiding package-level global variables - use constructor functions that return fresh instances and inject dependencies explicitly".
Also applies to: 91-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/containers.go` around lines 34 - 36, Remove the package-level mutable map emulatorContainerPorts and instead resolve ports inside the ContainerPort(...) function (or via an immutable config injected at construction); update ContainerPort to use a switch on EmulatorType (matching cases like EmulatorAWS) to return the appropriate port string, and delete the global map declaration (also remove/replace any other globals at lines ~91-97) so tests get no shared mutable state and port resolution is local to the function or supplied config.internal/runtime/runtime.go (1)
14-14: Keep runtime config adapter-agnostic.Line 14 encodes Docker-style port format (
"4566/tcp") in a runtime-wide struct. Consider splitting into neutral fields (e.g.,ContainerPort+Protocol) so each runtime adapter formats ports as needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtime/runtime.go` at line 14, The struct field ContainerPort currently encodes a Docker-style value ("4566/tcp"); change the runtime config to use neutral fields (e.g., ContainerPortNumber (string or int) and ContainerPortProtocol (string)) instead of a single formatted string, update any code paths that read/write ContainerPort to construct the adapter-specific formatted port (for example Docker adapter should format as fmt.Sprintf("%s/%s", ContainerPortNumber, ContainerPortProtocol)), and update parsing/validation in functions that consume ContainerPort (referencing the ContainerPort field in internal/runtime/runtime.go and any runtime adapter methods) so each adapter is responsible for formatting/serializing the final port representation.
🤖 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/config/containers.go`:
- Around line 34-36: Remove the package-level mutable map emulatorContainerPorts
and instead resolve ports inside the ContainerPort(...) function (or via an
immutable config injected at construction); update ContainerPort to use a switch
on EmulatorType (matching cases like EmulatorAWS) to return the appropriate port
string, and delete the global map declaration (also remove/replace any other
globals at lines ~91-97) so tests get no shared mutable state and port
resolution is local to the function or supplied config.
In `@internal/runtime/runtime.go`:
- Line 14: The struct field ContainerPort currently encodes a Docker-style value
("4566/tcp"); change the runtime config to use neutral fields (e.g.,
ContainerPortNumber (string or int) and ContainerPortProtocol (string)) instead
of a single formatted string, update any code paths that read/write
ContainerPort to construct the adapter-specific formatted port (for example
Docker adapter should format as fmt.Sprintf("%s/%s", ContainerPortNumber,
ContainerPortProtocol)), and update parsing/validation in functions that consume
ContainerPort (referencing the ContainerPort field in
internal/runtime/runtime.go and any runtime adapter methods) so each adapter is
responsible for formatting/serializing the final port representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 94c90055-933a-4689-8351-b5333507cfce
📒 Files selected for processing (5)
internal/config/containers.gointernal/container/start.gointernal/runtime/docker.gointernal/runtime/runtime.gotest/integration/start_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/start_test.go
|
@coderabbitai recheck |
3 participants
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.