Fix Redis container to accept input command #1155
Conversation
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @ziggornif, yes please include the redis-stack command change in this PR |
cristianrgreco
added
bug
Same as redis image with input command See issue testcontainers#1150
|
Hi @cristianrgreco, I just made the modification for redis-stack as well. The REDIS_ARGS variable now contains the merge of the user's values with the default values. I kept the current solution of creating the variable via |
| ]; | ||
| if (this.imageName.image.includes("redis-stack")) { | ||
| const existingRedisArgs = | ||
| (this.createOpts.Env || []).find((e) => e.startsWith("REDIS_ARGS="))?.split("=", 2)[1] || ""; |
There was a problem hiding this comment.
Potential bug here ?.split("=", 2)[1], if the value is undefined, then calling [1] will throw TypeError, would need to be ?.[1]
Also please use ?? instead of || for specifying default values.
I would use this.environment instead of this.createOps.Env to simplify this (don't need to split strings).
| // } | ||
| }); | ||
|
|
||
| it("should start redis with custom command", async () => { |
There was a problem hiding this comment.
The redis containers aren't stopped at the end of the tests. Let's see if we can follow the existing await using syntax to have this handled automatically for us
| const mergedRedisArgs = [existingRedisArgs, ...redisArgs].filter(Boolean).join(" "); | ||
|
|
||
| // remove existing REDIS_ARGS to avoid duplicates | ||
| this.createOpts.Env = (this.createOpts.Env || []).filter((e) => !e.startsWith("REDIS_ARGS=")); |
There was a problem hiding this comment.
Instead of directly mutating this.createOpts.Env, it would be better to invoke withEnvironment
cristianrgreco
added
the
changes requested
2 participants
Hi,
Here's a small fix to resolve the Redis container startup issue with custom commands (#1150).
I noticed a similar issue with the redis-stack image that also overrides
this.createOpts.Env.Would you prefer I include this fix in the same PR, or should I create a separate issue to track it first?