Add Clickhouse module #648
Conversation
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Thanks for your contribution, @tomershafir ! I've left some comments.
| if (this.hostConfigPathXml) | ||
| this.withCopyFilesToContainer([{source: this.hostConfigPathXml, target: CONFIG_PATH_XML, mode: CONFIG_FILE_MODE}]); | ||
| if (this.hostConfigPathYaml) | ||
| this.withCopyFilesToContainer([{source: this.hostConfigPathYaml, target: CONFIG_PATH_YAML, mode: CONFIG_FILE_MODE}]); |
There was a problem hiding this comment.
Although, those are very convenient, sometimes, can confuse to users when having multiple configurations. I'm not a ClickHouse expert, so, for now my suggestion would be to do not support those OOTB but rather have examples using the withCopyFilesToContainer as part of the documentation.
|
|
||
| function createClickhouseContainerHttpClient(container: StartedClickhouseContainer) { | ||
| return createClient({ | ||
| host: `http://${container.getHost()}:${container.getHostPorts().http}`, |
There was a problem hiding this comment.
is this should be returned by the container implementation for easy access?
There was a problem hiding this comment.
I think the client should be a different module rather than part the container abstraction
it also couples the dev to a dep he may not want (even though it may make sense to couple because its an official client), for example qryn project doesnt use this client atm
| public getHostPorts(): {http: number; native: number} { | ||
| return {http: this.httpHostPort, native: this.nativeHostPort} | ||
| } |
There was a problem hiding this comment.
I think the urls should be returned instead of the ports.
|
|
||
| function createClickhouseContainerHttpClient(container: StartedClickhouseContainer) { | ||
| return createClient({ | ||
| host: container.getHttpUrl("http"), |
There was a problem hiding this comment.
Due to the current support the getHttpUrl() can return the http protocol. If, at some point in the future, https is supported another method/fn can be added or revisited
cristianrgreco
added
enhancement
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@tomershafir please add your module under modules add file https://github.com/testcontainers/testcontainers-node/blob/main/mkdocs.yml. Otherwise it will be missing in the documentation. |
|
done, @cristianrgreco hey, can you merge? |
|
Thanks @tomershafir, please address the linting issues. The security alerts could also be solved by replacing |
|
done |
| public getNativeUrl(schema: string): string { | ||
| return this.toUrl(schema, this.hostNativePort) | ||
| } |
There was a problem hiding this comment.
can you provide an example of this please? or better add a test :)
There was a problem hiding this comment.
as clickhouse-js doesnt support native protocol I removed the js methods. Added https one
tomershafir
force-pushed
the
feat/clickhouse-module
branch
from
972304e
to
1e24e78
Compare
|
Thanks @eddumelendez 🙂 Will merge when the build passes |
|
@tomershafir This test is failing: |
Add basic clickhouse module