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

fix: Improved Oracle DB module #363

Merged
merged 23 commits into from
Mar 31, 2024
Merged

Conversation

gvenzl
Copy link
Contributor

@gvenzl gvenzl commented Jun 11, 2023

Hi,

I took the liberty to improve the Oracle DB module for Testcontainers Python. The PR has several enhancements:

  • Leveraging oracledb thin Python driver
    • This makes Oracle DB tests on CI/CD now possible too
  • Usage of gvenzl/oracle-free image with the latest and greatest Oracle DB version
  • DB version independent readiness check
  • Support for various gvenzl/oracle-free image features (ORACLE_DATABASE, APP_USER, APP_USER_PASSWORD, etc)
  • Tests for Oracle DB for the various combinations

Ideally, some more documentation on how the Container is supposed to be used would be handy but I couldn't really find a good example of how such a ReadMe should be structured.
Any things are gladly appreciated!

gvenzl added 18 commits June 10, 2023 19:47
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
and not dbname

Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
Signed-off-by: gvenzl <gerald.venzl@gmail.com>
@gvenzl
Copy link
Contributor Author

gvenzl commented Jun 12, 2023

Looking at the failed job, I'm fairly certain that this was a strange, potentially race condition on that individual execution of the test.
All other Oracle DB tests with different Python versions pass, and so did they over at https://github.com/gvenzl/testcontainers-python/actions/runs/5236983249

I have seen situations where Docker Hub refuses to have images pulled if the pull frequency is too high.
While I can only guess that something like that may have happened here, it would be great if an administrator could rerun that failed job.

My guess is that it will execute just fine and if not, it will be a clear indicator that there is something off with that particular test that needs fixing.

@tillahoffmann
Copy link
Collaborator

Yes, looks like it. I've restarted the run. Would it be possible to improve timeout handling so the tests fail earlier? The current setup ran for a few hours before bailing: https://github.com/testcontainers/testcontainers-python/actions/runs/5237034596/jobs/9470709201?pr=363#step:7:291

@gvenzl
Copy link
Contributor Author

gvenzl commented Jun 13, 2023

Hey @tillahoffmann,

Thanks a lot, it did go through this time!

Happy to increase the timeout if you could point me in the right direction.
I didn't deviate from the default 120s and according to the output, that was supposedly used:

FAILED oracle/tests/test_oracle.py::test_docker_run_oracle_with_system_password - TimeoutError: Wait time (120s) exceeded for _connect(args: (), kwargs: {}). Exception: Wait time (120s) exceeded for get_exposed_port(args: (1521,), kwargs: {}). Exception: Port mapping for container 11fd19cb8765cff8effcb3dac33c31bad52c2e5abe7cb5be8c7a5db6c18e73cf and port 1521 is not available

I suspect something went wrong under the covers with spinning up the container altogether, given the Exception: Port mapping for container. I'm not sure what else I'm supposed to do here as I think setting a different timeout won't solve this particular issue (given that it says it used the default 120s), or did I get something wrong here?

@gvenzl
Copy link
Contributor Author

gvenzl commented Jul 5, 2023

Hey @tillahoffmann, just wanted to check in on that one and see how you want to proceed.

Thx,

Signed-off-by: gvenzl <gerald.venzl@gmail.com>
@gvenzl
Copy link
Contributor Author

gvenzl commented Sep 13, 2023

Hey @tillahoffmann,

I've cleared the merge conflict and pumped up the oracledb driver version.
Could you please approve the workflows?

Signed-off-by: Gerald Venzl <gerald.venzl@gmail.com>
Signed-off-by: Gerald Venzl <gerald.venzl@gmail.com>
Signed-off-by: Gerald Venzl <gerald.venzl@gmail.com>
@thuyng-ing
Copy link

following! Is there any update on this? thanks!

@alexanderankin
Copy link
Collaborator

did some exploratory work here - main...alexanderankin:testcontainers-python:gvenzl_main

@alexanderankin alexanderankin added community-feat feature but its a community module so we wont bump tc core for it and removed 👀 requires attention labels Mar 26, 2024
@alexanderankin alexanderankin changed the title Improved Oracle DB module fix: Improved Oracle DB module Mar 31, 2024
@alexanderankin
Copy link
Collaborator

the original pull request which was opened last june had a hidden feature, which was a merged PR, which was also separately merged into testcontainers-python. additionally, there are several commits dealing only with setup.py and other removed files, which made for an interesting rebase. lastly, i removed the option to start it with a random password as that is unusable (youd have to open the logs and get the password out of there. at any rate, the oracle module is up to date again!

@alexanderankin alexanderankin merged commit 6e6d8e3 into testcontainers:main Mar 31, 2024
23 of 25 checks passed
@alexanderankin
Copy link
Collaborator

the original commits are here for reference - https://github.com/testcontainers/testcontainers-python/commits/gvenzl/oracle-free

alexanderankin pushed a commit that referenced this pull request Apr 1, 2024
🤖 I have created a release *beep* *boop*
---


##
[4.3.0](testcontainers-v4.2.0...testcontainers-v4.3.0)
(2024-04-01)


### Features

* **client:** Add custom User-Agent in Docker client as
`tc-python/&lt;version&gt;`
([#507](#507))
([dd55082](dd55082))


### Bug Fixes

* Add CassandraContainer
([#476](#476))
([507e466](507e466))
* add chroma container
([#515](#515))
([0729bf4](0729bf4))
* Add Weaviate module
([#492](#492))
([90762e8](90762e8))
* **cassandra:** make cassandra dependency optional/test-only
([#518](#518))
([bddbaeb](bddbaeb))
* **core:** allow setting docker command path for docker compose
([#512](#512))
([63fcd52](63fcd52))
* **google:** add support for Datastore emulator
([#508](#508))
([3d891a5](3d891a5))
* Improved Oracle DB module
([#363](#363))
([6e6d8e3](6e6d8e3))
* inconsistent test runs for community modules
([#497](#497))
([914f1e5](914f1e5))
* **kafka:** Add redpanda testcontainer module
([#441](#441))
([451d278](451d278))
* **kafka:** wait_for_logs in kafka container to reduce lib requirement
([#377](#377))
([909107b](909107b))
* **keycloak:** container should use dedicated API endpoints to
determine container readiness
([#490](#490))
([2e27225](2e27225))
* **nats:** Client-Free(ish) NATS container
([#462](#462))
([302c73d](302c73d))
* **new:** add a new Docker Registry test container
([#389](#389))
([0f554fb](0f554fb))
* pass doctests, s/doctest/doctests/, run them in gha,
s/asyncpg/psycopg/ in doctest, fix keycloak flakiness: wait for first
user
([#505](#505))
([545240d](545240d))
* pass updated keyword args to Publisher/Subscriber client in
google/pubsub
[#161](#161)
([#164](#164))
([8addc11](8addc11))
* Qdrant module
([#463](#463))
([e8876f4](e8876f4))
* remove accidentally added pip in dev dependencies
([#516](#516))
([dee20a7](dee20a7))
* **ryuk:** Enable Ryuk test suite. Ryuk image 0.5.1 -&gt; 0.7.0. Add
RYUK_RECONNECTION_TIMEOUT env variable
([#509](#509))
([472b2c2](472b2c2))

---
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>
@gvenzl
Copy link
Contributor Author

gvenzl commented Apr 2, 2024

Thanks a lot for merging this, @alexanderankin!

@alexanderankin
Copy link
Collaborator

@gvenzl thank you for your contribution!

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 feat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants