-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
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 Graphite |
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
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
@@ -22,97 +24,106 @@ | |||
* 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
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
exclusive
method 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