Skip to content

Conversation

@MitulShah1
Copy link
Contributor

@MitulShah1 MitulShah1 commented Apr 10, 2025

This PR adds AeroSpike as a module for

What does this PR do?

Why is it important?

Related issues

@stevenh @eddumelendez would appreciate review PR

@MitulShah1 MitulShah1 requested a review from a team as a code owner April 10, 2025 15:38
@netlify
Copy link

netlify bot commented Apr 10, 2025

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 1515488
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/67f90081bdb87d00088e3aba
😎 Deploy Preview https://deploy-preview-3094--testcontainers-go.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.

@mdelapenya
Copy link
Member

@MitulShah1 before moving on with the review, did you know that this module already exists as a community module? https://github.com/ajeetdsouza/testcontainers-aerospike-go

What would be the interest of moving it under here?

@MitulShah1
Copy link
Contributor Author

@MitulShah1 before moving on with the review, did you know that this module already exists as a community module? https://github.com/ajeetdsouza/testcontainers-aerospike-go

What would be the interest of moving it under here?

I thing this version belongs to official testcontainers-go, what you mention is individual developed.

@mdelapenya
Copy link
Member

I know. That module is listed in the modules catalog https://testcontainers.com/modules/aerospike/?language=go

Is there anything blocking you from using it?

@MitulShah1
Copy link
Contributor Author

MitulShah1 commented Apr 10, 2025

I can use that module, no problem.
People try to find in existing code base if exist or not then go for different option. I did same i tried but didn't find on GitHub for aerospike, there is no major implementation in my code, if you merge ok or Thanks for pointing out i will use that in my test case 😀

@mdelapenya
Copy link
Member

Not opposed to merge this into the main repo, just wanted to know the details

@MitulShah1
Copy link
Contributor Author

No worries, Thanks i just checked github codebase, next time i'll take care to check official doc as well, if it's listed or not. Meanwhile Closing PR

@MitulShah1 MitulShah1 closed this Apr 10, 2025
@MitulShah1
Copy link
Contributor Author

@mdelapenya I just close PR but when i start work, i saw your comment on https://github.com/ajeetdsouza/testcontainers-aerospike-go/pulls isnt it easy to manage here?

At the time I started working on this module, the feature wasn't available in the main repository. I identified the need based on project requirements and decided to proceed with development to avoid delays and to keep my work unblocked.

My implementation is aligned with current architecture and standards, and it has already been tested. I believe it offers value by addressing the issue early, and we can either merge it directly or integrate parts of it with the main repo’s version if there are complementary benefits.

I’m open to discussing how we can consolidate both efforts efficiently, but the intention was to be proactive and keep momentum going.

@mdelapenya
Copy link
Member

Let's reopen an start the discussion and review here. Thanks for pointing out

@mdelapenya mdelapenya reopened this Apr 11, 2025
@mdelapenya mdelapenya changed the title feat: Added Aerospike module feat(aerospike): add Aerospike module Apr 11, 2025
@MitulShah1
Copy link
Contributor Author

@mdelapenya thanks for review, suggestion changes are done.

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

Found some more issues, although I think we are very ready to be in good shape. Thanks for working on this!

@mdelapenya mdelapenya self-assigned this Apr 11, 2025
@mdelapenya mdelapenya added the enhancement New feature or request label Apr 11, 2025
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

We broke the docs site in the last commit. I can help you out with that adding commits on top of yours if needed

Copy link
Contributor

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Late to the party but there's a few bugs which need fixing.

Copy link
Contributor

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Looking good, just the one bug due to renamed error I think.

Copy link
Contributor

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for quick turnaround on fixes most appreciated 👏

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @MitulShah1 !

Once merged, could you please update the modules catalog here? https://github.com/testcontainers/community-module-registry/

@mdelapenya mdelapenya merged commit 751e94a into testcontainers:main Apr 11, 2025
16 checks passed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Apr 14, 2025
* main: (91 commits)
  chore(deps): bump github/codeql-action from 3.28.13 to 3.28.15 (testcontainers#3097)
  chore(deps): bump golang.org/x/crypto from 0.31.0 to 0.37.0 (testcontainers#3098)
  feat(aerospike): add Aerospike module (testcontainers#3094)
  security(compose): upgrade github.com/docker/compose/v2 to fix security vulnerability (testcontainers#3095)
  feat: add more functional options to the modules API (testcontainers#3070)
  chore(deps): bump golang.org/x/net in /modules/arangodb (testcontainers#3087)
  feat: add arangodb module (testcontainers#3083)
  chore(deps): bump actions/upload-artifact from 4.6.0 to 4.6.2 (testcontainers#3086)
  chore(deps): bump SonarSource/sonarqube-scan-action from 5.0.0 to 5.1.0 (testcontainers#3085)
  feat: add socat container (testcontainers#3071)
  fix(mssql): reduce flakiness in tests (testcontainers#3084)
  chore: bump golangci-lint to v2 (testcontainers#3082)
  chore(gcloud): deprecate old gcp containers, creating subpackages for them (testcontainers#3063)
  fix(mongodb): replica set initialization & connection handling (testcontainers#2984)
  chore(deps): bump docker/setup-docker-action from 4.2.0 to 4.3.0 (testcontainers#3077)
  chore(deps): bump github/codeql-action from 3.28.12 to 3.28.13 (testcontainers#3078)
  chore(deps): bump tj-actions/changed-files from 45.0.4 to 46.0.3 (testcontainers#3076)
  docs: add dependabot configuration (testcontainers#3074)
  chore(deps): replace `golang.org/x/exp/slices` with stdlib (testcontainers#3075)
  fix(dind): use docker image load (testcontainers#3073)
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Apr 15, 2025
* main:
  chore(deps): bump golang.org/x/crypto in /modules/aerospike (testcontainers#3105)
  chore(ci): run codeql on the modified modules (testcontainers#3103)
  docs: fix reference to container types in Run function (testcontainers#3102)
  chore(deps): bump github.com/golang-jwt/jwt/v5 in /modules/pulsar (testcontainers#3101)
  chore(pulsar): bump github.com/apache/pulsar-client-go from 0.10.0 to 0.14.0 (testcontainers#3100)
  chore(clickhouse): bump github.com/ClickHouse/clickhouse-go/v2 from 2.20.0 to 2.34.0 in /modules/clickhouse (testcontainers#3099)
  chore(deps): bump github/codeql-action from 3.28.13 to 3.28.15 (testcontainers#3097)
  chore(deps): bump golang.org/x/crypto from 0.31.0 to 0.37.0 (testcontainers#3098)
  feat(aerospike): add Aerospike module (testcontainers#3094)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants