chore(valkey): use Run function #3438
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughRefactors modules/valkey/valkey.go to assemble container options (moduleOpts) and call testcontainers.Run. Moves TLS cert generation and file/cmd injection into options, adjusts wait strategies, updates argument helpers, and wraps option/application errors consistently. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Valkey as Valkey Module
participant TLS as TLS Helper
participant TC as testcontainers
Caller->>Valkey: Run(ctx, img, opts...)
Valkey->>Valkey: Build moduleOpts (WithExposedPorts, WithWaitStrategy, etc.)
alt TLS enabled
Valkey->>TLS: generate certs/keys
TLS-->>Valkey: cert files & tlsConfig
Valkey->>Valkey: append WithFiles + WithCmd/WithCmdArgs
Valkey->>Valkey: extend wait strategy for TLS
end
Valkey->>TC: Run(ctx, img, moduleOpts..., opts...)
TC-->>Valkey: container / error
alt success
Valkey-->>Caller: container handle + tlsConfig
else failure
Valkey-->>Caller: error ("run valkey: %w")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/valkey/valkey.go (1)
160-163: Config file mode should be non-executable.0755 isn’t necessary for a config file; prefer 0644.
Apply this diff:
- cf := testcontainers.ContainerFile{ + cf := testcontainers.ContainerFile{ HostFilePath: configFile, ContainerFilePath: defaultConfigFile, - FileMode: 0o755, + FileMode: 0o644, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/valkey/valkey.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/valkey/valkey.go (3)
modules/valkey/options.go (1)
Option(22-22)options.go (7)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithCmdArgs(470-475)WithFiles(524-529)WithWaitStrategy(366-368)WithCmd(462-467)CustomizeRequestOption(28-28)container.go (1)
ContainerFile(110-115)
⏰ 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). (3)
- GitHub Check: test (1.25.x, modules/valkey) / test: modules/valkey/1.25.x
- GitHub Check: test (1.24.x, modules/valkey) / test: modules/valkey/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
modules/valkey/valkey.go (8)
74-76: Good default: expose Valkey port via WithExposedPorts.Order (module defaults first, user overrides later) is correct.
133-134: Good: wait strategies applied via WithWaitStrategy.Consolidated wait configuration aligns with the Run flow.
141-143: Helpful error wrapping.Returning the (possibly) initialized container along with wrapped error is useful for diagnostics/cleanup.
165-176: WithConfigFile correctly preserves previously added args.Prepending valkey-server and the config file while retaining any existing args (e.g., TLS flags) is the right behavior.
Consider adding an integration test that combines WithConfigFile + TLS to assert the final command resembles:
valkey-server /usr/local/valkey.conf --tls-port 6379 --port 0 --tls-cert-file ...
183-184: Arg processing unified through processValkeyServerArgs.Keeps log/snapshot flags consistent regardless of prior command configuration.
Also applies to: 200-201
204-217: Confirm valkey-server image ENTRYPOINT
I couldn’t find a Dockerfile for the valkey-server image in this repo—please verify the container’s ENTRYPOINT/CMD behavior (e.g. that it execsvalkey-serverwhen invoked with args) to ensure WithCmd-only flows remain valid.
83-86: Remove duplicate port wait, use TrimSuffix, and tighten TLS key perms
- Remove the second
wait.ForListeningPort(valkeyPort)…in the TLS block (line 85).- Replace
strings.Replace(valkeyPort, "/tcp", "", 1)withstrings.TrimSuffix(valkeyPort, "/tcp")(line 96).- Optionally set
server.keyFileMode to0o640instead of0o644(currently at line 121) if ownership permits; verify thatWithFilescreates/tlswith the correct directory, permissions, and ownership.
64-72: Option filtering is unnecessary Option.Customize is a NOOP, so forwarding Module Options into testcontainers.Run has no effect and filtering them only adds complexity.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
mdelapenya
force-pushed
the
use-run-claude-valkey
branch
from
b85c38c to
ab45829
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
modules/valkey/valkey.go (4)
78-86: Remove redundant port wait and avoid 10s per-strategy timeout (flaky in CI).
- Duplicate ForListeningPort in TLS branch (same port) is unnecessary.
- 10s per-strategy timeout is tight; rely on the global 60s deadline or increase.
Apply:
- waitStrategies := []wait.Strategy{ - wait.ForListeningPort(valkeyPort).WithStartupTimeout(time.Second * 10), - wait.ForLog("* Ready to accept connections"), - } + waitStrategies := []wait.Strategy{ + wait.ForListeningPort(valkeyPort), + wait.ForLog("* Ready to accept connections"), + } @@ - // wait for the TLS port to be available - waitStrategies = append(waitStrategies, wait.ForListeningPort(valkeyPort).WithStartupTimeout(time.Second*10)) + // TLS uses the same port; base port wait above suffices
105-107: Prefer using processValkeyServerArgs for TLS args (don’t rely on image entrypoint).If Cmd is empty, WithCmdArgs relies on the image’s ENTRYPOINT/CMD. Using the existing helper makes this robust and consistent with other options.
- testcontainers.WithCmdArgs(cmds...), + testcontainers.CustomizeRequestOption(func(req *testcontainers.GenericContainerRequest) error { + return processValkeyServerArgs(req, cmds) + }),
141-143: Include image in the error for easier debugging.- return c, fmt.Errorf("run valkey: %w", err) + return c, fmt.Errorf("run valkey (%s): %w", img, err)
154-158: Config file doesn’t need execute bit; use 0644.- FileMode: 0o755, + FileMode: 0o644,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/valkey/valkey.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/valkey/valkey.go (3)
modules/valkey/options.go (1)
Option(22-22)options.go (7)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithCmdArgs(470-475)WithFiles(524-529)WithWaitStrategy(366-368)WithCmd(462-467)CustomizeRequestOption(28-28)container.go (1)
ContainerFile(110-115)
⏰ 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: test (1.24.x, modules/valkey) / test: modules/valkey/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
modules/valkey/valkey.go (1)
64-73: Option implements ContainerCustomizer; no action required Option’s Customize method satisfies testcontainers.ContainerCustomizer and ensures all custom options are applied.Likely an incorrect or invalid review comment.
| testcontainers.ContainerFile{ | ||
| Reader: bytes.NewReader(serverCert.KeyBytes), | ||
| ContainerFilePath: "/tls/server.key", | ||
| FileMode: 0o644, | ||
| }), |
There was a problem hiding this comment.
server.key should be 0600 (private key).
Current 0644 makes the key world-readable in the container. Tighten permissions.
- testcontainers.ContainerFile{
- Reader: bytes.NewReader(serverCert.KeyBytes),
- ContainerFilePath: "/tls/server.key",
- FileMode: 0o644,
- }),
+ testcontainers.ContainerFile{
+ Reader: bytes.NewReader(serverCert.KeyBytes),
+ ContainerFilePath: "/tls/server.key",
+ FileMode: 0o600,
+ }),📝 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.
| testcontainers.ContainerFile{ | |
| Reader: bytes.NewReader(serverCert.KeyBytes), | |
| ContainerFilePath: "/tls/server.key", | |
| FileMode: 0o644, | |
| }), | |
| testcontainers.ContainerFile{ | |
| Reader: bytes.NewReader(serverCert.KeyBytes), | |
| ContainerFilePath: "/tls/server.key", | |
| FileMode: 0o600, | |
| }), |
🤖 Prompt for AI Agents
In modules/valkey/valkey.go around lines 118 to 122, the server private key file
is being written into the container with mode 0644 which makes it
world-readable; change the FileMode for "/tls/server.key" to 0600 (use Go octal
0o600) so the key is only readable/writable by the owner. Ensure the value is
the correct octal literal and rebuild/tests to verify permissions.
1 participant
What does this PR do?
Use the Run function in valkey
Why is it important?
Migrate modules to the new API, improving consistency and leveraging the latest testcontainers functionality.
Related issues