Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi @JensvandeWiel thanks for submitting the new module. Do you think you can extract the valkey module changeset to a separate PR? There are changes to the go modules that should not apply to the module creation. Or you think the changes are related? If not, I'd suggest sending two PRs: one for the module, one for the dependency updates |
|
Hey, I thought I needed to update dependencies as it says in the contribution guide. I can probably remove the dependencies updates from the other modules if you want |
Yes please, probably the contribution guides are misleading, talking about the core, not the modules. In any case, I'd prefer one PR for the valkey module, and one for the deps 🙏 |
|
Ok, ill do that later today |
|
Ok I've removed the other changes, I found that I still need to change the client to use the valkey client, so ill do that first, so please wait with merging. |
|
Ok, I have updated the repo with the main branch and switched Client to Valkey-go. It's ready to go 👍 |
|
Cool thank you @JensvandeWiel! Did you read the module contribution guide here? It seems the code generation tool was not used, so the PR just includes the module code, but nothing about the docs, and the GH action, etc. Could you please run the generator adding your code on top of the generated scaffolding? 🙏 |
|
Oh, I'm sorry ill do it soon! |
|
Ok, I've added it! |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
@JensvandeWiel I left a few comments regarding linting. Could you address them?
Other than that, LGTM thank you so much for your contribution! 🚀
I'll merge once the comments are addressed and the CI passes (it already pass locally, so I'm confident it will).
Cheers!
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
|
Ok, I've added the changes and updated it with main branch. |
|
The render URL looks pretty nice! https://deploy-preview-2639--testcontainers-go.netlify.app/modules/valkey/#container-methods well done! |
|
Module is live in the modules catalog: testcontainers/community-module-registry#62 |
* main: feat: add grafana-lgtm module (#2660) Added valkey module (#2639) fix: container.Endpoint and wait.FortHTTP to use lowest internal port (#2641) chore: test cleanups (#2657) docs: fix compilation of examples (#2656) feat: add custom container registry substitutor (#2647) fix: couchbase containers intermittently hang on startup (#2650) chore(deps): bump Ryuk to 0.8.1 (#2648) fix: retry on label error (#2644) perf: optimise docker authentication config lookup (#2646)
* main: chore: print Docker Info labels in banner (testcontainers#2681) fix: incorrect parsing of exposedPorts in readiness check (testcontainers#2658) feat: add grafana-lgtm module (testcontainers#2660) Added valkey module (testcontainers#2639) fix: container.Endpoint and wait.FortHTTP to use lowest internal port (testcontainers#2641)
2 participants
What does this PR do?
It adds a dedicated module For valkey using the Redis module as base.
Why is it important?
Valkey can work with the Redis module, but since it is not the same project and since it can change compared to Redis it should have it's own module.