-
Notifications
You must be signed in to change notification settings - Fork 296
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: support influxdb #413
feat: support influxdb #413
Conversation
…nnection URL and service version; add unit tests
…documentation and the build process
…ted modules; enhance the automated test; declared component dependencies
in the meantime, I created the https://github.com/Purecontrol/testcontainers-python-influxdb to publish the https://pypi.org/project/testcontainers-python-influxdb/ package. |
please review: main...alexanderankin:testcontainers-python:feat/support-influxdb fixed a test here: |
hello @alexanderankin Thank you for the feedback on this topic 🙏 I am sorry, I am not sure to understand what you want me to do 😞 most of the links you sent me are links to create pull requests on your github account. Don't you want to create them first and then ask me to review them? Or would you like me to "cherry-pick" the changes on the PR I opened? |
Sorry I should have clarified - can you confirm that I did not make any
unintentional changes while rebasing? I would force push but I think it's
impolite without first asking
…On Fri, Mar 8, 2024, 3:51 AM Luc Sorel-Giffo ***@***.***> wrote:
hello @alexanderankin <https://github.com/alexanderankin>
Thank you for the feedback on this topic 🙏
I am sorry, I am not sure to understand what you want me to do 😞 most of
the links you sent me are links to create pull requests on your github
account. Don't you want to create them first and then ask me to review them?
Or would you like me to "cherry-pick" the changes on the PR I opened?
—
Reply to this email directly, view it on GitHub
<#413 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACECGJC4X4O4SGHUZCUGDQLYXF3ZPAVCNFSM6AAAAABB4WT432VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBVGI4TEOJRGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
If it's okay with you I will replace your 9 original commits with the seven commits which result from rebasing onto the new structure (two of them edited files which no longer exist, such as setup.py) |
thank you for clarifying and asking.
Yes, that is fine 👍 |
there is just one change that should be done: those lines (main...alexanderankin:testcontainers-python:feat/support-influxdb_adjust#diff-d05626efd4466a0f9ddf3973c13071ea2e534dc69a295e68cc9fc3e5a5541e0fR115-R145) should be removed because they force the parent Instead, both InfluxDb1Container and InfluxDb2Container classes are independent and have their own Once you have force-pushed, your changes, I can add a commit to remove those unnecessary lines and imports, if you like? |
Yes feel free to yourself as well as I will be at work. Since the commits
are pushed to GitHub public repo you have access to everything
…On Fri, Mar 8, 2024, 4:17 AM Luc Sorel-Giffo ***@***.***> wrote:
there is just one change that should be done: those lines (
main...alexanderankin:testcontainers-python:feat/support-influxdb_adjust
#diff-d05626efd4466a0f9ddf3973c13071ea2e534dc69a295e68cc9fc3e5a5541e0fR115-R145
<main...alexanderankin:testcontainers-python:feat/support-influxdb_adjust#diff-d05626efd4466a0f9ddf3973c13071ea2e534dc69a295e68cc9fc3e5a5541e0fR115-R145>)
should be removed because they force the main container class to import the
dependencies for InfluxDB version 1.x and 2.x, whereas users may use either
one or the other, but it is not a good idea to force them to install the
dependencies of both versions.
Instead, both InfluxDb1Container
<main...alexanderankin:testcontainers-python:feat/support-influxdb_adjust#diff-bd64360629a8b2ac551283a6d10500059bfb36e17340e484bc1d8fd0322c81ccR47-R63>
and InfluxDb2Container
<main...alexanderankin:testcontainers-python:feat/support-influxdb_adjust#diff-e5cc1b8aa3516d4eff82f2f95976f6aeeb926b75a12a7d8aea92a2a043a287aaR82-R102>
classes are independent and have their own get_client() method.
Once you have force-pushed, your changes, I can add a commit to remove
those unnecessary lines and imports, if you like?
—
Reply to this email directly, view it on GitHub
<#413 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACECGJENSR7XT7VIWJBLY53YXF6Y3AVCNFSM6AAAAABB4WT432VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBVGMZTCNJTHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
381d1f3
to
4ff505d
Compare
feel free to layer your changes on this i rebased to address the part about the client_v1 and v2 so its in your repo now where you can work with it normally. feel free to run the "pre-commit run -a" to test linting locally as well |
thank you for fixing the I don't have anything to add, but I can easily fix the linting issues if you like. |
I see that testcontainers-python now uses pre-commit to run black for formatting and ruff for linting. I would suggest to use ruff-format as a drop-in replacement for black, but maybe in another issue? |
…expected code formatting
I pushed the commit related to the pre-commit hooks and the lint hooks now passes 👍 I am just wondering how to launch the influxdb unit tests locally. I believe the steps are:
|
|
Poetry install -E arangodb
Poetry shell
Pytest modules/arangodb
Poetry run pytest/modules/arangodb
…On Fri, Mar 8, 2024, 6:49 AM Luc Sorel-Giffo ***@***.***> wrote:
I pushed the commit related to the pre-commit hooks and the lint hooks now
passes 👍
I am just wondering how to launch the influxdb unit tests locally. I
believe the steps are:
1. install the optional dependencies for influxdb only
2. run the tests for influxdb only
I am not sure how to do that, now that the codebase has been moved to
the modules folder
—
Reply to this email directly, view it on GitHub
<#413 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACECGJDM2M2AFRUO65VB4TLYXGQUVAVCNFSM6AAAAABB4WT432VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBVGU2TONRVGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I pushed a fix a unit test. If the pipeline succeeds, I believe we're good to approve and merge the PR 😃 |
I would suggest to add the documentation about running the automated tests of a given module either in the root |
Ok lgtm then |
I think a maintainer needs to trigger the CI workflow to validate all the changes (see https://github.com/testcontainers/testcontainers-python/actions) and the PR. Moreover, I cannot approve the PR myself. |
I resolved a conflict on the |
all checks have passed 🎉 thank you 🙏 Is there an action that I should do for the merge? |
🤖 I have created a release *beep* *boop* --- ## [4.2.0](testcontainers-v4.1.0...testcontainers-v4.2.0) (2024-03-24) ### Features * support influxdb ([#413](#413)) ([13742a5](13742a5)) ### Bug Fixes * **arangodb:** tests to pass on ARM CPUs - change default image to 3.11.x where ARM image is published ([#479](#479)) ([7b58a50](7b58a50)) * **core:** DinD issues [#141](#141), [#329](#329) ([#368](#368)) ([b10d916](b10d916)) * **core:** raise an exception when docker compose fails to start [#258](#258) ([#485](#485)) ([d61af38](d61af38)) * **core:** use auto_remove=True with reaper instance ([#499](#499)) ([274a400](274a400)) * **docs:** update the non-existent main.yml badge ([#493](#493)) ([1d10c1c](1d10c1c)) * Fix the return type of `DockerContainer.get_logs` ([#487](#487)) ([cd72f68](cd72f68)) * **keycloak:** tests on aarch64, use image from [jboss -> quay], change supported version [16+ -> 18+] ([#480](#480)) ([5758310](5758310)) * **postgres:** doctest ([#473](#473)) ([c9c6f92](c9c6f92)) * read the docs build works again ([#496](#496)) ([dfd1781](dfd1781)) * readthedocs build - take 1 ([#495](#495)) ([b3b9901](b3b9901)) --- 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>
Thanks to the team for this great testing utility 🙏 This PR brings the possibility to spawn a Docker instance of InfluxDB (timeseries-oriented database) for integration testing. This PR supports both 1.x and 2.x versions of InfluxDB, which rely on different query languages and require the use of different Python client libraries. Therefore: - there is a root `InfluxDbContainer` class in `testcontainers/influxdb.py` for the common mechanisms (health check, connection url, etc.) - there are 2 separate classes, one for each InfluxDB major version: `InfluxDb1Container` class in `testcontainers/influxdb1/__init__.py`, `InfluxDb2Container` in `testcontainers/influxdb2/__init__.py` that are meant to be used by the testcontainers users. Each one has its own `.get_client()` method involving the dedicated Python client library Unit and integration tests (meant to be examples for the testcontainers end-users) are provided in the PR.
🤖 I have created a release *beep* *boop* --- ## [4.2.0](testcontainers/testcontainers-python@testcontainers-v4.1.0...testcontainers-v4.2.0) (2024-03-24) ### Features * support influxdb ([testcontainers#413](testcontainers#413)) ([13742a5](testcontainers@13742a5)) ### Bug Fixes * **arangodb:** tests to pass on ARM CPUs - change default image to 3.11.x where ARM image is published ([testcontainers#479](testcontainers#479)) ([7b58a50](testcontainers@7b58a50)) * **core:** DinD issues [testcontainers#141](testcontainers#141), [testcontainers#329](testcontainers#329) ([testcontainers#368](testcontainers#368)) ([b10d916](testcontainers@b10d916)) * **core:** raise an exception when docker compose fails to start [testcontainers#258](testcontainers#258) ([testcontainers#485](testcontainers#485)) ([d61af38](testcontainers@d61af38)) * **core:** use auto_remove=True with reaper instance ([testcontainers#499](testcontainers#499)) ([274a400](testcontainers@274a400)) * **docs:** update the non-existent main.yml badge ([testcontainers#493](testcontainers#493)) ([1d10c1c](testcontainers@1d10c1c)) * Fix the return type of `DockerContainer.get_logs` ([testcontainers#487](testcontainers#487)) ([cd72f68](testcontainers@cd72f68)) * **keycloak:** tests on aarch64, use image from [jboss -> quay], change supported version [16+ -> 18+] ([testcontainers#480](testcontainers#480)) ([5758310](testcontainers@5758310)) * **postgres:** doctest ([testcontainers#473](testcontainers#473)) ([c9c6f92](testcontainers@c9c6f92)) * read the docs build works again ([testcontainers#496](testcontainers#496)) ([dfd1781](testcontainers@dfd1781)) * readthedocs build - take 1 ([testcontainers#495](testcontainers#495)) ([b3b9901](testcontainers@b3b9901)) --- 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>
Thanks to the team for this great testing utility 🙏
This PR brings the possibility to spawn a Docker instance of InfluxDB (timeseries-oriented database) for integration testing. This PR supports both 1.x and 2.x versions of InfluxDB, which rely on different query languages and require the use of different Python client libraries. Therefore:
InfluxDbContainer
class intestcontainers/influxdb.py
for the common mechanisms (health check, connection url, etc.)InfluxDb1Container
class intestcontainers/influxdb1/__init__.py
,InfluxDb2Container
intestcontainers/influxdb2/__init__.py
that are meant to be used by the testcontainers users. Each one has its own.get_client()
method involving the dedicated Python client libraryUnit and integration tests (meant to be examples for the testcontainers end-users) are provided in the PR.
I declared the 2 Python client libraries as dependencies in
setup.py
so that the continuous integration of testcontainers can be launched. I wonder whether it implies that both clients will be installed by the testcontainers end-users although they are likely to need only one: either the 1.x client or the 2.x client. Installing the 2 but using only 1 works, but it slows down the installation and test processes. I am open to advice from the maintainers for that 🙏