- Notifications
You must be signed in to change notification settings - Fork 4.9k
make exclusive containers first class citizens #34892
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
make exclusive containers first class citizens #34892
Conversation
| The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
| This stack of pull requests is managed by Graphite. Learn more about stacking. Join @stephane-airbyte and the rest of your teammates on |
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
| seems simpler this way |
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Outdated Show resolved Hide resolved
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Outdated Show resolved Hide resolved
0499a70 to 707f5dd Compare 707f5dd to b56578e Compare Coverage report for source-postgres
|
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Show resolved Hide resolved
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Outdated Show resolved Hide resolved
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Outdated Show resolved Hide resolved
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Show resolved Hide resolved
| import org.testcontainers.utility.DockerImageName; | ||
| | ||
| /** | ||
| * ContainerFactory is the companion interface to {@link TestDatabase} for providing it with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment needs adjusting
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Show resolved Hide resolved
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Outdated Show resolved Hide resolved
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Show resolved Hide resolved
.../airbyte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java Outdated Show resolved Hide resolved
postamar left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better, thanks for doing this. Considering that mssql is still broken on master feel free to adopt these changes in another source in order to get this to merge.
Can you port over the log message colouring from my PR also? It's actually useful. The commit is 22702c6
b56578e to df37742 Compare 93e2258 to 559087f Compare 559087f to dd40f34 Compare | /publish-java-cdk
|
Co-authored-by: Marius Posta <marius@airbyte.io>
Co-authored-by: Marius Posta <marius@airbyte.io>
Co-authored-by: Marius Posta <marius@airbyte.io>
Co-authored-by: Marius Posta <marius@airbyte.io>
Co-authored-by: Marius Posta <marius@airbyte.io>

Very heavily lifted from #34865
Originally, the db-source's ContainerFactory test fixture was built under the assumption that testcontainers could always be shared by multiple tests. In practice it turns out that that's not always the case, especially for CDC tests where the CDC state is at the RDBMS level. For instance, in mysql, there's only one binlog.
Recently we struggled to debug an issue because an unshared testcontainer didn't forward its logs to the logging back-end. This is done by default for shared containers, but was forgotten for an unshared container in a CDC test.
Rather than peppering these tests with the suitable logging incantations, this PR adds an
exclusivemethod to ContainerFactory which is like the existing shared except that, well, the container is not shared. This way testcontainers are provisioned in the same way regardless of whether they're shared or not.While doing that, I took the opportunity to simplify the existing code