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: support influxdb #413

Merged
merged 13 commits into from
Mar 20, 2024

Conversation

lucsorel
Copy link
Contributor

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 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 🙏

@lucsorel
Copy link
Contributor Author

@alexanderankin
Copy link
Collaborator

alexanderankin commented Mar 8, 2024

@alexanderankin
Copy link
Collaborator

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

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?

@alexanderankin
Copy link
Collaborator

alexanderankin commented Mar 8, 2024 via email

@alexanderankin
Copy link
Collaborator

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)

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

thank you for clarifying and asking.

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)

Yes, that is fine 👍

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

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 InfluxDbContainer container class (which is not meant to be used directly by end users of testcontainers) 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 and InfluxDb2Container 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?

@alexanderankin
Copy link
Collaborator

alexanderankin commented Mar 8, 2024 via email

@alexanderankin
Copy link
Collaborator

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

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

thank you for fixing the client_v1 and _v2.

I don't have anything to add, but I can easily fix the linting issues if you like.

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

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?

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

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

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

poetry install -E influxdb seems to do the job for the dependencies

@alexanderankin
Copy link
Collaborator

alexanderankin commented Mar 8, 2024 via email

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

poetry run pytest modules/influxdb worked indeed 🙏

I pushed a fix a unit test. If the pipeline succeeds, I believe we're good to approve and merge the PR 😃

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

I would suggest to add the documentation about running the automated tests of a given module either in the root README.md or in modules/README.md. What do you think?

@alexanderankin
Copy link
Collaborator

Ok lgtm then

@lucsorel
Copy link
Contributor Author

lucsorel commented Mar 8, 2024

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.

@lucsorel
Copy link
Contributor Author

I resolved a conflict on the poetry.lock lockfile. The CI workflows need to be relaunched

@lucsorel
Copy link
Contributor Author

all checks have passed 🎉 thank you 🙏

Is there an action that I should do for the merge?

@alexanderankin alexanderankin merged commit 13742a5 into testcontainers:main Mar 20, 2024
9 checks passed
alexanderankin pushed a commit that referenced this pull request Mar 24, 2024
🤖 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>
bearrito pushed a commit to bearrito/testcontainers-python that referenced this pull request Mar 30, 2024
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.
bearrito pushed a commit to bearrito/testcontainers-python that referenced this pull request Mar 30, 2024
🤖 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 -&gt; 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants