Conversation
silv-io
requested review from
anisaoshafi and
carole-lavillonniere
as code owners
| // If Volume is set in the config, it is returned as-is. Otherwise, a default is computed | ||
| // from os.UserCacheDir()/lstk/volume/<container-name>. | ||
| func (c *ContainerConfig) VolumeDir() (string, error) { | ||
| if c.Volume != "" { |
Collaborator
There was a problem hiding this comment.
question: isn't this never empty since we set a default value in internal/config/config.go line 27? Maybe let's remove the viper default so we generate the default path here?
Member
Author
There was a problem hiding this comment.
it can be empty if someone does a custom config and forgets to set it. I wanted it to be set in the default config though so that it's more discoverable. I can also remove it if needed though.
Member
Author
There was a problem hiding this comment.
I'll remove it for now and then we can revisit later. Maybe discoverability is better solved by just having a big all options example config file.
carole-lavillonniere
approved these changes
Mar 17, 2026
Collaborator
carole-lavillonniere
left a comment
carole-lavillonniere
left a comment
There was a problem hiding this comment.
Looks good, just one question
silv-io
force-pushed
the
silv-io/flc-473
branch
from
a01b6bb to
f28e1c4
Compare
📝 WalkthroughWalkthroughThis pull request introduces volume mounting support for containerized environments. The changes add a configurable Volume field to ContainerConfig with a VolumeDir() method that determines the host directory for persistence, integrate volume mounting into container startup, and add corresponding test assertions and cleanup logic. Changes
Sequence DiagramsequenceDiagram
participant Config as ContainerConfig
participant Start as Container Start
participant FS as File System
participant Docker as Docker Engine
Start->>Config: Read Volume field
Start->>Config: Call VolumeDir()
Config->>Config: Check if Volume is set
alt Volume set
Config->>Config: Return Volume path
else Volume not set
Config->>FS: Get user cache directory
Config->>Config: Construct default path
end
Config-->>Start: Return volumeDir
Start->>FS: Create volume directory (MkdirAll)
FS-->>Start: Directory created
Start->>Docker: Mount volumeDir to /var/lib/localstack
Docker-->>Start: Container ready with volume
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 Tip You can get early access to new features in CodeRabbit.Enable the |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/container/start.go`:
- Around line 100-108: The code is creating the volume directory (calling
c.VolumeDir() and os.MkdirAll) before containers are filtered by
selectContainersToStart, which can mutate disk or fail even when no start should
occur; move the volume dir resolution and os.MkdirAll call into the actual start
path executed after selectContainersToStart returns (e.g., inside the start
routine that handles selected containers), and only call c.VolumeDir(),
os.MkdirAll and append the runtime.BindMount once you have confirmed the
container is selected to start; update references where binds is constructed so
it is built post-selection rather than in the pre-selection block.
In `@test/integration/awsconfig_test.go`:
- Around line 25-34: The cleanup Docker command in the t.Cleanup closure can
leave dotfiles (hidden files) behind because it runs rm -rf /d/*; update the
cleanup to remove all entries including hidden ones by changing the container
command invoked in the t.Cleanup function that references volumeDir (the
exec.Command call) to use a safe pattern that deletes both dotfiles and normal
files (for example use a shell delete that targets both * and .*, or use find /d
-mindepth 1 -delete) so the root-owned hidden files are removed before Go's
TempDir cleanup runs.
In `@test/integration/start_test.go`:
- Around line 186-189: The test's bind-path check is brittle because
hasBindTarget currently splits bind strings on ":" which breaks on Windows
drive-letter paths; update hasBindTarget to parse bind specs from the end (e.g.,
split on ":" but treat the last segment as optional mode and the second-to-last
segment as the container target, or use strings.LastIndex logic) so it correctly
extracts the container path even when the host path contains ":"; ensure this
logic is used by the volume mount test in start_test.go and that hasBindTarget
still handles both "host:container" and "host:container:mode" forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7c176cd3-8bd2-4a87-ae41-579b2b500303
📒 Files selected for processing (4)
internal/config/containers.gointernal/container/start.gotest/integration/awsconfig_test.gotest/integration/start_test.go
Comment on lines
+100
to
+108
| volumeDir, err := c.VolumeDir() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := os.MkdirAll(volumeDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create volume directory %s: %w", volumeDir, err) | ||
| } | ||
| binds = append(binds, runtime.BindMount{HostPath: volumeDir, ContainerPath: "/var/lib/localstack"}) | ||
|
|
There was a problem hiding this comment.
Volume directory creation happens before start eligibility is decided.
Lines 100-106 run before selectContainersToStart, so lstk start can fail or mutate disk even when the emulator is already running (or port checks fail).
💡 Suggested fix (defer mkdir to actual start path)
- volumeDir, err := c.VolumeDir()
- if err != nil {
- return err
- }
- if err := os.MkdirAll(volumeDir, 0755); err != nil {
- return fmt.Errorf("failed to create volume directory %s: %w", volumeDir, err)
- }
- binds = append(binds, runtime.BindMount{HostPath: volumeDir, ContainerPath: "/var/lib/localstack"})
+ volumeDir, err := c.VolumeDir()
+ if err != nil {
+ return err
+ }
+ binds = append(binds, runtime.BindMount{HostPath: volumeDir, ContainerPath: "/var/lib/localstack"}) func startContainers(ctx context.Context, rt runtime.Runtime, sink output.Sink, containers []runtime.ContainerConfig) error {
for _, c := range containers {
+ for _, b := range c.Binds {
+ if b.ContainerPath == "/var/lib/localstack" {
+ if err := os.MkdirAll(b.HostPath, 0755); err != nil {
+ return fmt.Errorf("failed to create volume directory %s: %w", b.HostPath, err)
+ }
+ }
+ }
output.EmitStatus(sink, "starting", c.Name, "")
containerID, err := rt.Start(ctx, c)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/container/start.go` around lines 100 - 108, The code is creating the
volume directory (calling c.VolumeDir() and os.MkdirAll) before containers are
filtered by selectContainersToStart, which can mutate disk or fail even when no
start should occur; move the volume dir resolution and os.MkdirAll call into the
actual start path executed after selectContainersToStart returns (e.g., inside
the start routine that handles selected containers), and only call
c.VolumeDir(), os.MkdirAll and append the runtime.BindMount once you have
confirmed the container is selected to start; update references where binds is
constructed so it is built post-selection rather than in the pre-selection
block.
Comment on lines
+25
to
+34
| // Runs before t.TempDir() cleanup (LIFO order). The emulator runs as root | ||
| // inside the container, so files it writes into the volume are root-owned on | ||
| // Linux. Go's TempDir cleanup can't delete them, so we use a Docker container | ||
| // to remove them first. | ||
| t.Cleanup(func() { | ||
| volumeDir := filepath.Join(tmpHome, ".cache", "lstk", "volume") | ||
| if _, err := os.Stat(volumeDir); err == nil { | ||
| _ = exec.Command("docker", "run", "--rm", "-v", volumeDir+":/d", "alpine", "sh", "-c", "rm -rf /d/*").Run() | ||
| } | ||
| }) |
There was a problem hiding this comment.
Cleanup command can leave hidden files behind.
At Line 32, rm -rf /d/* won’t remove dotfiles; hidden root-owned files can still cause temp-dir cleanup failures.
💡 Suggested fix
t.Cleanup(func() {
volumeDir := filepath.Join(tmpHome, ".cache", "lstk", "volume")
if _, err := os.Stat(volumeDir); err == nil {
- _ = exec.Command("docker", "run", "--rm", "-v", volumeDir+":/d", "alpine", "sh", "-c", "rm -rf /d/*").Run()
+ cmd := exec.Command("docker", "run", "--rm", "-v", volumeDir+":/d", "alpine", "sh", "-c", "find /d -mindepth 1 -delete")
+ if out, err := cmd.CombinedOutput(); err != nil {
+ t.Errorf("failed to cleanup volume dir %s: %v (%s)", volumeDir, err, strings.TrimSpace(string(out)))
+ }
}
})📝 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.
Suggested change
| // Runs before t.TempDir() cleanup (LIFO order). The emulator runs as root | |
| // inside the container, so files it writes into the volume are root-owned on | |
| // Linux. Go's TempDir cleanup can't delete them, so we use a Docker container | |
| // to remove them first. | |
| t.Cleanup(func() { | |
| volumeDir := filepath.Join(tmpHome, ".cache", "lstk", "volume") | |
| if _, err := os.Stat(volumeDir); err == nil { | |
| _ = exec.Command("docker", "run", "--rm", "-v", volumeDir+":/d", "alpine", "sh", "-c", "rm -rf /d/*").Run() | |
| } | |
| }) | |
| // Runs before t.TempDir() cleanup (LIFO order). The emulator runs as root | |
| // inside the container, so files it writes into the volume are root-owned on | |
| // Linux. Go's TempDir cleanup can't delete them, so we use a Docker container | |
| // to remove them first. | |
| t.Cleanup(func() { | |
| volumeDir := filepath.Join(tmpHome, ".cache", "lstk", "volume") | |
| if _, err := os.Stat(volumeDir); err == nil { | |
| cmd := exec.Command("docker", "run", "--rm", "-v", volumeDir+":/d", "alpine", "sh", "-c", "find /d -mindepth 1 -delete") | |
| if out, err := cmd.CombinedOutput(); err != nil { | |
| t.Errorf("failed to cleanup volume dir %s: %v (%s)", volumeDir, err, strings.TrimSpace(string(out))) | |
| } | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/awsconfig_test.go` around lines 25 - 34, The cleanup Docker
command in the t.Cleanup closure can leave dotfiles (hidden files) behind
because it runs rm -rf /d/*; update the cleanup to remove all entries including
hidden ones by changing the container command invoked in the t.Cleanup function
that references volumeDir (the exec.Command call) to use a safe pattern that
deletes both dotfiles and normal files (for example use a shell delete that
targets both * and .*, or use find /d -mindepth 1 -delete) so the root-owned
hidden files are removed before Go's TempDir cleanup runs.
Comment on lines
+186
to
+189
| t.Run("volume mount", func(t *testing.T) { | ||
| assert.True(t, hasBindTarget(inspect.HostConfig.Binds, "/var/lib/localstack"), | ||
| "expected volume bind mount to /var/lib/localstack, got: %v", inspect.HostConfig.Binds) | ||
| }) |
There was a problem hiding this comment.
Volume-mount assertion is brittle with Windows-style bind paths.
At Line 187, this relies on hasBindTarget, which splits bind strings on ":"; that can misparse drive-letter host paths and produce false negatives.
💡 Suggested fix (make bind parsing tolerant to host-path colons)
func hasBindTarget(binds []string, containerPath string) bool {
for _, b := range binds {
parts := strings.Split(b, ":")
- if len(parts) >= 2 && parts[1] == containerPath {
+ if len(parts) < 2 {
+ continue
+ }
+ // host:container or host:container:options; host may itself contain ":" (e.g. Windows drive letter)
+ if parts[len(parts)-1] == containerPath || parts[len(parts)-2] == containerPath {
return true
}
}
return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/start_test.go` around lines 186 - 189, The test's bind-path
check is brittle because hasBindTarget currently splits bind strings on ":"
which breaks on Windows drive-letter paths; update hasBindTarget to parse bind
specs from the end (e.g., split on ":" but treat the last segment as optional
mode and the second-to-last segment as the container target, or use
strings.LastIndex logic) so it correctly extracts the container path even when
the host path contains ":"; ensure this logic is used by the volume mount test
in start_test.go and that hasBindTarget still handles both "host:container" and
"host:container:mode" forms.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mount a host directory into
/var/lib/localstackfor caching and opt-in persistence (PERSISTENCE=1).Changes
Volumefield toContainerConfigandVolumeDir()method for resolving the host pathos.UserCacheDir()/lstk/volume/<container-name>(e.g.~/Library/Caches/lstk/volume/localstack-aws)volumefield in[[containers]]configvolumein default config written on first runos.MkdirAll) before container startvolumeDir:/var/lib/localstackTests
TestStartCommandSetsUpContainerCorrectlyverifying the volume bind mount targets/var/lib/localstackRelated