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

feat: Adding support for Cassandra and Scylla #167

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

fruch
Copy link
Contributor

@fruch fruch commented Dec 30, 2021

This add the support for those cassandra based dbs
and their drivers, cassandra-driver, scylla-driver

Ref: https://cassandra.apache.org/
Ref: https://www.scylladb.com/
Ref: https://pypi.org/project/cassandra-driver/
Ref: https://pypi.org/project/scylla-driver/

This add the support for those cassandra based dbs
and their drivers, cassandra-driver, scylla-driver

Ref: https://cassandra.apache.org/
Ref: https://www.scylladb.com/
Ref: https://pypi.org/project/cassandra-driver/
Ref: https://pypi.org/project/scylla-driver/
@fruch
Copy link
Contributor Author

fruch commented Dec 30, 2021

@yakimka
Copy link
Collaborator

yakimka commented Apr 5, 2022

@fruch hello. Can you update this PR?

@tillahoffmann
Copy link
Collaborator

Thanks for the additions and apologies for the slow reply. A few things to get this integrated:

  • Rebase on master.
  • Regenerate requirement files.
  • Add cassandra and scylla to the test-components in .github/workflows/main.yml.

@fruch
Copy link
Contributor Author

fruch commented Sep 18, 2022

Thanks for the additions and apologies for the slow reply. A few things to get this integrated:

  • Rebase on master.
  • Regenerate requirement files.
  • Add cassandra and scylla to the test-components in .github/workflows/main.yml.

I've rebased and added the test to CI

but the building of requirements is failing:

There are incompatible versions in the resolved dependencies:
  protobuf<4.0.0dev (from google-cloud-pubsub==1.7.2->testcontainers===dev->-r requirements.in (line 1))
  protobuf<5.0.0dev,>=3.20.1 (from google-api-core[grpc]==2.10.1->google-cloud-pubsub==1.7.2->testcontainers===dev->-r requirements.in (line 1))
  protobuf<5.0.0dev,>=3.15.0 (from googleapis-common-protos[grpc]==1.56.4->google-api-core[grpc]==2.10.1->google-cloud-pubsub==1.7.2->testcontainers===dev->-r requirements.in (line 1))
  protobuf>=4.21.3 (from grpcio-status==1.49.0->google-api-core[grpc]==2.10.1->google-cloud-pubsub==1.7.2->testcontainers===dev->-r requirements.in (line 1))

I think pinning of google-cloud-pubsub is problematic, and google packages are conflicting each other.
I'm trying to unpin it, and regenerate (but it quite a long process, meanwhile it worked for python3.7)

@fruch
Copy link
Contributor Author

fruch commented Sep 18, 2022

@yakimka @tillahoffmann waiting for approval for running the CI

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@ca65a91). Click here to learn what that means.

❗ Current head dd6520d differs from pull request most recent head 1114f4d. Consider uploading reports for the commit 1114f4d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #167   +/-   ##
=======================================
  Coverage        ?   87.81%           
=======================================
  Files           ?       32           
  Lines           ?      886           
  Branches        ?       58           
=======================================
  Hits            ?      778           
  Misses          ?       78           
  Partials        ?       30           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tillahoffmann
Copy link
Collaborator

We may have to get #209 merged first to get around the google dependency issue.

@tillahoffmann
Copy link
Collaborator

tillahoffmann commented Sep 19, 2022

I have a relatively strong preference to remove the cache for two reasons. First, it's more code to maintain. Second, I don't think the caching will help in most real-world scenarios.

I appreciate that runtime is different across different machines, but getting a single image takes about 4ms on my machine. Using the cache, getting a single image takes about 300ms. It'll of course be quicker the next time images are fetched, but we'd have to use at least 75 containers per test suite to make the cache worthwhile. To have a delay of one second over the cached version, we'd have to run about 325 containers. I'd argue that for a test suite that runs more than 300 docker containers a delay of one second is negligible. But let me know if I misunderstood/miscalculated.

@fruch
Copy link
Contributor Author

fruch commented Sep 19, 2022

We may have to get #209 merged first to get around the google dependency issue.

Seems that just removing the pinning helped

@fruch
Copy link
Contributor Author

fruch commented Sep 19, 2022

I have a relatively strong preference to remove the cache for two reasons. First, it's more code to maintain. Second, I don't think the caching will help in most real-world scenarios.

I appreciate that runtime is different across different machines, but getting a single image takes about 4ms on my machine. Using the cache, getting a single image takes about 300ms. It'll of course be quicker the next time images are fetched, but we'd have to use at least 75 containers per test suite to make the cache worthwhile. To have a delay of one second over the cached version, we'd have to run about 325 containers. I'd argue that for a test suite that runs more than 300 docker containers a delay of one second is negligible. But let me know if I misunderstood/miscalculated.

Didn't understand where you are going with the calculation here, for me getting a docker image out of dockerhub can take a minute or two. I was more talking about pip-compile that in case of conflicts can download all the versions of a package to try solving it, (I know it, cause I'm using it in in one of my projects in my day job),

And as you said I don't think it's worth while for a library to maintain such a lock file. even that tool like poetry for example are doing just that. (also can be slow and annoying)

@tillahoffmann
Copy link
Collaborator

Sorry, I posted on the wrong thread. 🤦‍♂️

@fruch
Copy link
Contributor Author

fruch commented Sep 20, 2022

We may have to get #209 merged first to get around the google dependency issue.

@tillahoffmann would it be o.k. to take the code changes from #209, and test it in tandam ?

@spicy-sauce
Copy link
Contributor

Hi,
What's the status of this PR? Do you plan to merge it to the master anytime soon?

@fruch I see there are conflicts that should be resolved, is it something that you should handle?

Thanks,
Yossi

@fruch
Copy link
Contributor Author

fruch commented Dec 13, 2022

Hi,
What's the status of this PR? Do you plan to merge it to the master anytime soon?

@fruch I see there are conflicts that should be resolved, is it something that you should handle?

I guess I could, I've almost forgotten about this one, it's almost a whole year since I've opened this one.

Thanks,
Yossi

@spicy-sauce
Copy link
Contributor

spicy-sauce commented Dec 13, 2022

I guess I could, I've almost forgotten about this one, it's almost a whole year since I've opened this one.

Thanks!

@tillahoffmann can we please push it once @fruch resolves the conflicts? This seems like a great addition to testcontainers!

@fruch
Copy link
Contributor Author

fruch commented Dec 13, 2022

I guess I could, I've almost forgotten about this one, it's almost a whole year since I've opened this one.

Thanks!

@tillahoffmann can we please push it once @fruch resolves the conflicts? This seems like a great addition to testcontainers!

BTW @spicy-sauce can you share a bit what do you need it for ? Cassandra or Scylla ? And which type of testing it's gonna be part of ?

@spicy-sauce
Copy link
Contributor

spicy-sauce commented Dec 13, 2022

BTW @spicy-sauce can you share a bit what do you need it for ? Cassandra or Scylla ? And which type of testing it's gonna be part of ?

At this point I need it for Cassandra as I'm going to implement such a block in DataYoga and would love to use it for the testing part.

Read more here:
https://datayoga-io.github.io/datayoga/

@fruch
Copy link
Contributor Author

fruch commented Dec 13, 2022

BTW @spicy-sauce can you share a bit what do you need it for ? Cassandra or Scylla ? And which type of testing it's gonna be part of ?

At this point I need it for Cassandra as I'm going to implement such a block in DataYoga and would love to use it for the testing part.

Read more here: https://datayoga-io.github.io/datayoga/

looks interesting, ping me when you have it running with Cassandra or Scylla, maybe adding it in our https://university.scylladb.com/

anyhow I'm the maintainer of python driver fork
https://github.com/scylladb/python-driver

if you have question you can reach me at fruch@scylladb.com

@fruch
Copy link
Contributor Author

fruch commented Dec 13, 2022

@tillahoffmann #209 didn't seemed to be merged

Anyhow I've rebased again, and rebuild the requirements files again.

Can now it be run in CI and merged ?

@fruch
Copy link
Contributor Author

fruch commented Dec 14, 2022

@tillahoffmann looks like I've missed updating the requriements files for 3.7, now it should run o.k.

@spicy-sauce
Copy link
Contributor

spicy-sauce commented Dec 16, 2022

Guys, can we please push this one? It's really a great addition and we would like to avoid another conflicts.
Thanks a lot!!!

@fruch
Copy link
Contributor Author

fruch commented Dec 19, 2022

@tillahoffmann @yakimka it's now passes all the tests...

@spicy-sauce
Copy link
Contributor

Is there anything preventing this from being merged?

In the meanwhile, I'd solve the current conflicts.

@SergeyPirogov
Copy link
Collaborator

There is a issue with relases because I live in Kyiv and we are fighting with Russsia, so there is no time for the project

@alexanderankin
Copy link
Collaborator

alexanderankin commented Jan 14, 2024

@SergeyPirogov that very important and I wish for victory for Ukraine of course.

can we help with releases in the meantime? My username is daveankin on Pypi and if you add me as owner I will work with currently available maintainers to update this library and Pypi, without removing you from the project at all. I hope soon in the future you will have some time for the project and return but until then, please consider adding myself or someone from testcontainers/atomicjar for the meantime.

Thank you!

edit: I have accepted the invite to join on pypi

@guy9
Copy link

guy9 commented Jan 23, 2024

Also interested in this and happy to help

@alexanderankin
Copy link
Collaborator

alexanderankin commented Jan 23, 2024 via email

@alexanderankin
Copy link
Collaborator

alexanderankin commented Jan 23, 2024 via email

@alexanderankin
Copy link
Collaborator

alexanderankin commented Mar 9, 2024

hi @fruch,

i have backed up your commit as it exists today in here: dd6520d - please confirm its okay to rebase and push some commits to your branch to adjust to the new structure (#408)?

i see that you have enabled Maintainers are allowed to edit this pull request., so thank you for that, but i am in a habit of asking before i force push, so I appreciate your timely response (or we will figure out something to do for a cassandra implementation based off of this one, i suppose)

thanks,
David

@fruch
Copy link
Contributor Author

fruch commented Mar 10, 2024

hi @fruch,

i have backed up your commit as it exists today in here: dd6520d - please confirm its okay to rebase and push some commits to your branch to adjust to the new structure (#408)?

i see that you have enabled Maintainers are allowed to edit this pull request., so thank you for that, but i am in a habit of asking before i force push, so I appreciate your timely response (or we will figure out something to do for a cassandra implementation based off of this one, i suppose)

thanks,
David

Go right ahead, ping me you are done, I would give it a look (can do that also after it merged)

@alexanderankin
Copy link
Collaborator

alexanderankin commented Mar 14, 2024

i dont think we actually want to merge this when it references an abandoned docker image for cassandra, so ive made some adjustments here - fruch/testcontainers-python@cassandra_scylla_support...testcontainers:testcontainers-python:cassandra_scylla_support_image_improvement

with these additional changes i think this is ready to merge

@alexanderankin alexanderankin added community-feat feature but its a community module so we wont bump tc core for it and removed needs-rebase labels Mar 26, 2024
@alexanderankin alexanderankin force-pushed the cassandra_scylla_support branch 2 times, most recently from a4f78b7 to 0b0711e Compare August 14, 2024 12:30
@alexanderankin
Copy link
Collaborator

rebased so this is ready to be merged

linking related - https://github.com/testcontainers/testcontainers-java/pull/8002/files

@alexanderankin alexanderankin changed the title Adding support for Cassandra and Scylla feat: Adding support for Cassandra and Scylla Aug 14, 2024
@alexanderankin alexanderankin merged commit 2d8bc11 into testcontainers:main Aug 14, 2024
13 of 14 checks passed
alexanderankin pushed a commit that referenced this pull request Aug 14, 2024
🤖 I have created a release *beep* *boop*
---


##
[4.8.0](testcontainers-v4.7.2...testcontainers-v4.8.0)
(2024-08-14)


### Features

* Adding support for Cassandra and Scylla
([#167](#167))
([2d8bc11](2d8bc11))
* **compose:** ability to retain volumes when using context manager
([#659](#659))
([e1e3d13](e1e3d13))
* **compose:** add ability to get docker compose config
([#669](#669))
([8c28a86](8c28a86))
* **core:** add ability to do OR & AND for waitforlogs
([#661](#661))
([b1453e8](b1453e8))
* **new:** Added AWS Lambda module
([#655](#655))
([9161cb6](9161cb6))
* refactor network setup
([#678](#678))
([d5de0aa](d5de0aa))


### Bug Fixes

* Add Db2 support
([#673](#673))
([1e43923](1e43923))
* bring back cassandra driver bc otherwise how does it get installed for
cassandra module test run?
([#680](#680))
([71c3a1a](71c3a1a))
* **rabbitmq:** add `vhost` as parameter to RabbitMqContainer
([#656](#656))
([fa2081a](fa2081a))
* **selenium:** add Arg/Options to api of selenium container
([#654](#654))
([e02c1b3](e02c1b3)),
closes
[#652](#652)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-feat feature but its a community module so we wont bump tc core for it 🚀 enhancement feat ✨ package: new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants