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 support for dynamodb-local #3557

Closed
wants to merge 5 commits into from

Conversation

fnobilia
Copy link

@fnobilia fnobilia commented Dec 4, 2020

Define a TestContainer using the official AWS DynamoDb local docker image as an alternative to Dynalite.

This new Testcontainer uses the latest version of AWS SDK, instead of the old AWS SDK currently used in the Dynalite container.

* @deprecated use {@link DynamoDbContainer (DockerImageName)} instead
*/
@Deprecated
public DynamoDbContainer() {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this constructor? can it be removed?

Copy link
Author

Choose a reason for hiding this comment

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

Nope, we don't. Fixed!

DockerImageName.parse("amazon/dynamodb-local");
private static final int MAPPED_PORT = 8000;

public DynamoDbContainer(String dockerImageName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor isn't needed. Can you remove it?

Copy link
Author

Choose a reason for hiding this comment

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

You're right this contractor is not needed. I added only because I found it in most of the other modules. Is there any convention suggesting I should keep it?

@bsideup
Copy link
Member

bsideup commented Dec 6, 2020

FYI DynamoDB local is already supported by LocalStackContainer.

As it is a general solution to many AWS services, do you still think we need a module?

@fnobilia
Copy link
Author

fnobilia commented Dec 6, 2020

@bsideup I thought a specific module could be more discoverable given that DynamoDB Local is the suggested way to run tests against DynamoDB in the official AWS documentation (reference). One more point is that this module uses the latest AWS Java SDK, while both Dynalite and LocalStackContainer uses the old one.
Another very small advantage is that the DynamoDB Local docker image is smaller than the LocalStack one.

@rnorth
Copy link
Member

rnorth commented Dec 8, 2020

TBH I think we should avoid adding another module for DynamoDB. As @bsideup mentioned, Localstack already supports it and we have the dynalite module too. Arguably, our dynalite module should be retired because it's old and I can't see it adding any value after so long.

For us, as volunteer maintainers, adding a new module is quite a commitment in terms of future maintenance (which is why we have an extensive checklist for it). Sadly in this case with the functionality being quite similar to existing modules, I think we really can't accept the contribution at the moment. Sorry if this is disappointing.

@fnobilia
Copy link
Author

fnobilia commented Dec 8, 2020

@rnorth @bsideup thanks for the feedback. I will close this PR.

Is there any plan to update AWS SDK in the LocalStackContainer, because #1442 seems to be closed? Currently, we have duplicated some code to not leak AWS SDK v1 in our project, which is not ideal. It would be better having native support coming from Testcontainers for AWS SDK 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants