Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Valkey module #906

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Add Valkey module #906

merged 2 commits into from
Feb 5, 2025

Conversation

mdodsworth
Copy link
Contributor

…ntainer

Adding support for Valkey. Container and tests are based largely on the existing redis setup.

Copy link

netlify bot commented Jan 29, 2025

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 840aac3
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/67a00636c3b7cb0008f69400
😎 Deploy Preview https://deploy-preview-906--testcontainers-node.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

]);
}
const startedContainer = new StartedValkeyContainer(await super.start(), this.password);
if (this.initialImportScriptFile) await this.importInitialData(startedContainer);
Copy link
Collaborator

@cristianrgreco cristianrgreco Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this is copied from the Redis container code, but this can be refactored by moving this line into the containerStarted callback (https://node.testcontainers.org/features/containers/#lifecycle-callbacks). Extra ❤️ if you update the Redis container code as well 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will work for ❤️s.
Sure thing, I'll fix them both up

@cristianrgreco cristianrgreco added enhancement New feature or request minor Backward compatible functionality labels Jan 30, 2025
@cristianrgreco cristianrgreco changed the title feat: adding valkey container, heavily based on the existing redis co… Add Valkey container Jan 30, 2025
@cristianrgreco cristianrgreco changed the title Add Valkey container Add Valkey module Jan 30, 2025
@mdodsworth
Copy link
Contributor Author

@cristianrgreco changes are ready for review. Thanks fro the input

@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Feb 4, 2025

Thanks @mdodsworth. I had a thought.

If Valkey is a drop-in replacement for Redis then could we get away with this?

class ValkeyContainer extends RedisContainer {
  constructor(image = "valkey/valkey:8.0") {
    super(image);
  }

  // maybe need to override the start method to return a `StartedValkeyContainer`, 
  // but even this just calls `super(...)` ?
}

EDIT: I think it may complicate things in terms of how the modules depend on each other.

@mdodsworth
Copy link
Contributor Author

Thanks @mdodsworth. I had a thought.

If Valkey is a drop-in replacement for Redis then could we get away with this?

class ValkeyContainer extends RedisContainer {
  constructor(image = "valkey/valkey:8.0") {
    super(image);
  }

  // maybe need to override the start method to return a `StartedValkeyContainer`, 
  // but even this just calls `super(...)` ?
}

EDIT: I think it may complicate things in terms of how the modules depend on each other.

Yeah, my feeling was that the functionality may eventually diverge, so having it be separate container would be worthwhile

@cristianrgreco cristianrgreco merged commit 15b62a1 into testcontainers:main Feb 5, 2025
197 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Backward compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants