chore(clickhouse): use Run function #3325
Conversation
Summary by CodeRabbit
WalkthroughRefactors ClickHouse module to use testcontainers.Run with container customizers, adds HTTP wait strategy, and post-run credential inspection. Extracts configuration and customization options (Zookeeper, init scripts, config files, credentials) into a new options.go file. Removes embedded templates and related logic from clickhouse.go. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant CH as ClickHouse Module
participant TC as testcontainers.Run
participant Ctr as Container
participant Docker as Docker Engine
Dev->>CH: Run(ctx, reqOpts...)
CH->>CH: Build moduleOpts (ports, env, wait)
CH->>TC: Run(ctx, image, moduleOpts...)
TC->>Docker: Create & start container
Docker-->>TC: Container running
TC-->>CH: Container handle
CH->>Ctr: Inspect()
Ctr-->>CH: Env vars (CLICKHOUSE_USER/PASSWORD/DB)
CH->>CH: Populate credentials from env/defaults
CH-->>Dev: ClickHouseContainer (ready)
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
🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
modules/clickhouse/options.go (3)
24-24: Template name inconsistency.The template is named "bootstrap.yaml" but the embedded file is
zk_config.xml.tpland the output is XML. Consider using a more descriptive name like "zookeeper.xml" to match the actual content format.- tpl, err := template.New("bootstrap.yaml").Parse(zookeeperConfigTpl) + tpl, err := template.New("zookeeper.xml").Parse(zookeeperConfigTpl)
37-64: Consider file permission and temp file lifecycle.Two observations:
- File permissions: The XML config file uses
0o755(executable), but0o644would be more appropriate for a configuration file.- Temp file cleanup: The temporary file created by
os.CreateTempis not explicitly removed and will remain on the host filesystem after the container starts. While this may be acceptable for short-lived test containers, consider documenting this behavior or implementing cleanup if desired.If you want to adjust permissions:
cf := testcontainers.ContainerFile{ HostFilePath: f.Name(), ContainerFilePath: "/etc/clickhouse-server/config.d/zookeeper_config.xml", - FileMode: 0o755, + FileMode: 0o644, }
127-136: Consider validating non-empty password.The documentation states the password "must not be empty or undefined," but the function doesn't enforce this. While the ClickHouse container image may reject an empty password at runtime, failing fast with a clear error message would improve the developer experience.
func WithPassword(password string) testcontainers.CustomizeRequestOption { return func(req *testcontainers.GenericContainerRequest) error { + if password == "" { + return fmt.Errorf("password must not be empty") + } req.Env["CLICKHOUSE_PASSWORD"] = password return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/clickhouse/clickhouse.go(1 hunks)modules/clickhouse/options.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
modules/clickhouse/clickhouse.go (4)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithEnv(75-85)WithWaitStrategy(366-368)wait/http.go (1)
NewHTTPStrategy(48-63)modules/cassandra/cassandra.go (1)
Run(73-106)container.go (1)
Container(41-73)
modules/clickhouse/options.go (3)
options.go (1)
CustomizeRequestOption(28-28)generic.go (1)
GenericContainerRequest(21-27)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/clickhouse) / test: modules/clickhouse/1.25.x
- GitHub Check: test (1.24.x, modules/clickhouse) / test: modules/clickhouse/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
modules/clickhouse/options.go (4)
1-20: LGTM!Package declaration, imports, template embedding, and type definition are correct.
66-82: LGTM!Init scripts correctly mapped with executable permissions. The use of
filepath.Baseensures clean filenames in the container.
116-125: LGTM!Database name configuration correctly sets the environment variable. The default value handling is documented and deferred to the container image.
138-152: LGTM!Username configuration correctly handles the empty string case by defaulting to
defaultUser. This is consistent with the documented behavior.modules/clickhouse/clickhouse.go (3)
70-81: LGTM!The migration to the
testcontainers.Runpattern with modular options is well-structured. The HTTP wait strategy on port 8123 provides reliable container readiness detection.
83-92: LGTM!Correctly appends user-provided customizers after module defaults and wraps errors appropriately. The nil check ensures the wrapper container is only created when
ctris valid.
94-122: Remove cleanup-on-error suggestion. Module Run functions (ClickHouse, ArangoDB, Kafka, etc.) uniformly return the container and error, delegating termination to callers (for example via testcontainers.CleanupContainer or explicit Terminate). Introducing self-termination here would break that established pattern.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
modules/clickhouse/options.go (2)
84-98: Overly permissive file mode for config file.The file permissions
0o755(line 92) are overly permissive for a configuration file. Config files should use0o644for appropriate read/write permissions without execute bits.This issue was previously flagged in past reviews but has not been addressed.
Apply this diff:
cf := testcontainers.ContainerFile{ HostFilePath: configFile, ContainerFilePath: "/etc/clickhouse-server/config.d/config.xml", - FileMode: 0o755, + FileMode: 0o644, }
100-114: Overly permissive file mode for config file.The file permissions
0o755(line 108) are overly permissive for a YAML configuration file. Config files should use0o644for appropriate read/write permissions without execute bits.This issue was previously flagged in past reviews but has not been addressed.
Apply this diff:
cf := testcontainers.ContainerFile{ HostFilePath: configFile, ContainerFilePath: "/etc/clickhouse-server/config.d/config.yaml", - FileMode: 0o755, + FileMode: 0o644, }
🧹 Nitpick comments (1)
modules/clickhouse/options.go (1)
22-35: Consider renaming template for clarity.The template is named
"bootstrap.yaml"but it renders zookeeper XML configuration. Consider using a more descriptive name like"zookeeper-config"to better reflect its purpose.- tpl, err := template.New("bootstrap.yaml").Parse(zookeeperConfigTpl) + tpl, err := template.New("zookeeper-config").Parse(zookeeperConfigTpl)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/clickhouse/options.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/clickhouse/options.go (3)
options.go (1)
CustomizeRequestOption(28-28)generic.go (1)
GenericContainerRequest(21-27)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/clickhouse) / test: modules/clickhouse/1.25.x
- GitHub Check: test (1.24.x, modules/clickhouse) / test: modules/clickhouse/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
modules/clickhouse/options.go (2)
66-82: LGTM!The function correctly mounts init scripts with executable permissions (
0o755), which is appropriate for scripts in/docker-entrypoint-initdb.d/that need to be executed by the container entrypoint.
116-152: LGTM. The option functions are well-implemented and thedefaultUserconstant is defined in modules/clickhouse/clickhouse.go (line 15).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
modules/clickhouse/options.go (3)
59-63: Overly permissive file mode for config file.The file mode
0o755grants execute permissions which are unnecessary for a configuration file. Config files should use0o644(read/write for owner, read-only for group and others).Apply this diff to use appropriate permissions:
cf := testcontainers.ContainerFile{ HostFilePath: f.Name(), ContainerFilePath: "/etc/clickhouse-server/config.d/zookeeper_config.xml", - FileMode: 0o755, + FileMode: 0o644, }
93-97: Overly permissive file mode for config file.The file mode
0o755grants execute permissions which are unnecessary for a configuration file. Config files should use0o644.Apply this diff:
cf := testcontainers.ContainerFile{ HostFilePath: configFile, ContainerFilePath: "/etc/clickhouse-server/config.d/config.xml", - FileMode: 0o755, + FileMode: 0o644, }
109-113: Overly permissive file mode for config file.The file mode
0o755grants execute permissions which are unnecessary for a YAML configuration file. Config files should use0o644.Apply this diff:
cf := testcontainers.ContainerFile{ HostFilePath: configFile, ContainerFilePath: "/etc/clickhouse-server/config.d/config.yaml", - FileMode: 0o755, + FileMode: 0o644, }
🧹 Nitpick comments (1)
modules/clickhouse/options.go (1)
25-25: Template name mismatch (minor).The template is named "bootstrap.yaml" but the embedded file is
zk_config.xml.tpl. While this doesn't affect functionality, using a consistent name would improve code clarity.Apply this diff to align the template name:
- tpl, err := template.New("bootstrap.yaml").Parse(zookeeperConfigTpl) + tpl, err := template.New("zk_config.xml.tpl").Parse(zookeeperConfigTpl)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/clickhouse/options.go(1 hunks)modules/clickhouse/options_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- modules/clickhouse/options_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
modules/clickhouse/options.go (3)
options.go (1)
CustomizeRequestOption(28-28)generic.go (1)
GenericContainerRequest(21-27)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/clickhouse) / test: modules/clickhouse/1.25.x
- GitHub Check: test (1.24.x, modules/clickhouse) / test: modules/clickhouse/1.24.x
- GitHub Check: Analyze (go)
This reverts commit 8839db7.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
modules/clickhouse/options.go (3)
37-67: Config file has overly permissive file mode.Line 61 sets
FileMode: 0o755(rwxr-xr-x) for the zookeeper config file. Config files don't require execute permissions;0o644(rw-r--r--) is more appropriate and follows the principle of least privilege.Apply this diff:
cf := testcontainers.ContainerFile{ HostFilePath: f.Name(), ContainerFilePath: "/etc/clickhouse-server/config.d/zookeeper_config.xml", - FileMode: 0o755, + FileMode: 0o644, }
103-117: Config file has overly permissive file mode.Line 111 sets
FileMode: 0o755for the YAML config file. Config files should use0o644(rw-r--r--) instead of0o755(rwxr-xr-x) as they don't require execute permissions.Apply this diff:
cf := testcontainers.ContainerFile{ HostFilePath: configFile, ContainerFilePath: "/etc/clickhouse-server/config.d/config.yaml", - FileMode: 0o755, + FileMode: 0o644, }
87-101: Config file has overly permissive file mode.Line 95 sets
FileMode: 0o755for the XML config file. Config files should use0o644(rw-r--r--) instead of0o755(rwxr-xr-x) as they don't require execute permissions.Apply this diff:
cf := testcontainers.ContainerFile{ HostFilePath: configFile, ContainerFilePath: "/etc/clickhouse-server/config.d/config.xml", - FileMode: 0o755, + FileMode: 0o644, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/clickhouse/options.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/clickhouse/options.go (3)
options.go (1)
CustomizeRequestOption(28-28)generic.go (1)
GenericContainerRequest(21-27)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.24.x, modules/clickhouse) / test: modules/clickhouse/1.24.x
- GitHub Check: test (1.25.x, modules/clickhouse) / test: modules/clickhouse/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
modules/clickhouse/options.go (2)
69-85: LGTM!The function correctly sets up init scripts with executable permissions (0o755), which is appropriate since these scripts are executed by the container's entrypoint.
119-155: No action needed:defaultUseris defined. ThedefaultUserconstant is declared in modules/clickhouse/clickhouse.go, so theWithUsernamefunction will compile correctly.
1 participant
What does this PR do?
Use Run function in the Clickhouse module, moving all options to its own file.
Why is it important?
Migrate modules to the new API
Related issues