#355 Matrix testing on CI#360
Conversation
C0urante
force-pushed
the
gh-355
branch
8 times, most recently
from
72ae30b to
69683f8
Compare
gunnarmorling
left a comment
gunnarmorling
left a comment
There was a problem hiding this comment.
This looks great! Two minor comments/suggestions inline.
| if (parsedDebeziumVersion.greaterOrEquals(SPANNER_CONNECTOR_MIN_VERSION)) { | ||
| spannerConnectorDescription = "source io.debezium.connector.spanner.SpannerConnector " + debeziumVersion; | ||
| } |
There was a problem hiding this comment.
This makes me think that we probably should move to the connect-base image (see #303), as it would avoid all this dance around new Debezium connectors added in specific versions. Separate issue though :)
There was a problem hiding this comment.
+1 for switching to connect-base, although we'd still end up doing most of the dancing here since the rest (besides the Spanner connector) concerns MM2 and the file connectors :)
There was a problem hiding this comment.
LOL, yeah, fair point. The other advantage would be faster test execution, as the plug-in path scanning in Connect feels surprisingly slow.
There was a problem hiding this comment.
| - name: 'Build' | ||
| run: ./mvnw -ntp -B --file pom.xml verify | ||
| env: | ||
| DEBEZIUM_TAG: ${{ matrix.debezium_tag }} |
There was a problem hiding this comment.
Instead of passing in the Debezium tag, could we make this to use the Kafka version (as that's what matters in the end for controlling which tests to run). E.g. when a test fails due to an unsupported API, we'd easily say that this is about, say, Kafka Connect 3.2, rather than some largely unrelated Debezium version. This would require to codify the Kafka-to-Debezium version mapping somewhere in the code which spins up Testcontainers.
There was a problem hiding this comment.
Good call, done! I've left in the latest stable and nightly images since they may help us catch breaking changes with Connect sooner, but can remove those too if they don't seem worth the effort.
| CONNECT_VERSION: ${{ matrix.connect_version }} | ||
|
|
||
| strategy: | ||
| fail-fast: false |
There was a problem hiding this comment.
This prevents all concurrent jobs from being cancelled as soon as the first one fails; docs here.
| kafkaConnect.ensureConnectorState(connector, Connector.State.RUNNING); | ||
| kafkaConnect.ensureConnectorTaskState(connector, 0, Connector.State.RUNNING); |
There was a problem hiding this comment.
This test failed in a prior CI run. In rare cases, we'd successfully submit the connector to the Connect cluster, and then before tasks for it could be brought up, run the get connectors command.
The calls to ensureConnectorState and (more importantly) ensureConnectorTaskState here ensure that the connector and its task have actually started before we examine the cluster using the CLI.
| } | ||
|
|
||
| @Test | ||
| @SkipIfConnectVersionIsOlderThan("3.2") |
There was a problem hiding this comment.
This syntax is so smooth and unobtrusive. Great idea, Gunnar!
There was a problem hiding this comment.
Ha yeah, thanks a lot for setting this up! We have something similar in Debezium, worked great there as well.
gunnarmorling
left a comment
gunnarmorling
left a comment
There was a problem hiding this comment.
Great improvement, thanks a lot! LGTM, merging.
2 participants
Fixes #355
Runs integration tests against a wide range of versions of Kafka Connect using the approach outlined in the linked issue.
Adds automatic CI runs against Kafka Connect versions 2.8.x through 3.5.x, along with the latest stable build and nightly build for the Debezium image we currently use for testing.
We can add more versions (probably going back no further than 2.0.x... really hope nobody's stuck on anything older than that 🤞) either in this PR or as a follow-up, whichever would be easier to review.