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.

Sign up for GitHub

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

Jump to bottom

fix(nats): Client-Free(ish) NATS container #462

Merged
merged 4 commits into from
Mar 31, 2024
Merged

fix(nats): Client-Free(ish) NATS container #462

merged 4 commits into from
Mar 31, 2024

Conversation

bearrito
Copy link
Contributor

@bearrito bearrito commented Mar 9, 2024

@totallyzen
Closed previous PR because of dirty history and bad-merge.

Based of this comment - #446 (comment)
I removed hard dependency on the NATS client.

This PR does not require a client to work or validate at a basic level.
However if a developer wants to use a client, there is test support for that.

bearrito
bearrito commented Mar 9, 2024
View reviewed changes
modules/nats/tests/test_nats.py Outdated
"""


NO_NATS_CLIENT = True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section will check if you have the client available, if so then certain tests are available. If not then those tests are skipped.

Copy link
Member

Choose a reason for hiding this comment

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

there's really no reason to exclude tests due to missing deps - just dont force a dep on the client code which is using the testcontainer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the correct way to indicate that with poetry?

Copy link
Member

@alexanderankin alexanderankin Mar 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What's the correct way to indicate that with poetry?

extremely painfully 😄

@alexanderankin
Copy link
Member

@bearrito for what its worth we havent decided what we are doing with the dependencies yet - i was going to merge your PR as is after releasing whatever other fixes we can fit before we add features (things that have to bump minor version)

@alexanderankin alexanderankin changed the title Client-Free(ish) NATS container feat(nats): Client-Free(ish) NATS container Mar 9, 2024
@bearrito
Copy link
Contributor Author

bearrito commented Mar 9, 2024

@alexanderankin No worries. If you guys decide to allow more dependencies. I can add to the extras and then remove the try/catch import

@bearrito
Copy link
Contributor Author

@alexanderankin I see the commit. Do you want me to resolve the merge conflict or are you guys still deciding on dependencies?

@alexanderankin alexanderankin added the community-feat feature but its a community module so we wont bump tc core for it label Mar 26, 2024
@totallyzen
Copy link
Collaborator

@bearrito my take at this point is that you can add it as a dev dependency - the point is more so to prevent enforcement of clients, not to make testing a hellscape.

I've reflected this in the latest changes on #460 as well, comment there instead on this if you agree!

@alexanderankin
Copy link
Member

what happened here? i am rebasing all the branches because too crazy otherwise

@bearrito
Copy link
Contributor Author

@alexanderankin I think I was in the middle of you doing that. What do you want me to do here?

@alexanderankin alexanderankin changed the title feat(nats): Client-Free(ish) NATS container fix(nats): Client-Free(ish) NATS container Mar 31, 2024
@alexanderankin
Copy link
Member

@bearrito no problem, i just merged all the newer ones, about to rebase this one and merge it. thanks for your contribution and support here. let me know if you want to have a final look over in a few mins before i push and hit merge.

@alexanderankin
Copy link
Member

need to fix the doctest still, one min

@alexanderankin alexanderankin merged commit 302c73d into testcontainers:main Mar 31, 2024
12 checks passed
@github-actions github-actions bot mentioned this pull request Mar 31, 2024
Merged
alexanderankin pushed a commit that referenced this pull request Apr 1, 2024
@github-actions
chore(main): release testcontainers 4.3.0 (#500)
f819c7a 
🤖 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/<version>`
([#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 -> 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderankin alexanderankin alexanderankin left review comments

Assignees
No one assigned
Labels
community-feat feature but its a community module so we wont bump tc core for it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bearrito @alexanderankin @totallyzen