Store protocol version from spec #16416
Conversation
github-actions
bot
added
area/api
|
The end to end tests are currently failing because we need to publish the test connectors. |
| @@ -2603,6 +2603,9 @@ components: | |||
| format: uri | |||
| icon: | |||
| type: string | |||
| protocolVersion: | |||
There was a problem hiding this comment.
Not sure what we have done in the past, but this has been named protocol_version in the protocol. Should we match the case with the field in the protocol or have we been using camel case traditionally here in the API definition even if the protocol uses lower underscore?
There was a problem hiding this comment.
I am sticking camel case for consistency.
Our casing is a bit inconsistent at the moment, but a lot of it is in the protocol which would make fixing it a breaking change. We should plan that clean up at some point.
| "2.1.1, 0.2.0", | ||
| "2.2.0, 0.2.1", | ||
| }) | ||
| void testCreateSourceSpec(final String dockerImageTag, final String expectedProtocolVersion) |
There was a problem hiding this comment.
It is not very clear about what is going to be feed in the parameter without looking a the doc. Am I the only one to feel that?
There was a problem hiding this comment.
Another option would be using a different source. I am not a fan of the extra link to follow to get to the test arguments.
@MethodSource("stringIntAndListProvider")
void testWithMultiArgMethodSource(String str, int num, List<String> list) {
assertEquals(5, str.length());
assertTrue(num >=1 && num <=2);
assertEquals(2, list.size());
}
static Stream<Arguments> stringIntAndListProvider() {
return Stream.of(
arguments("apple", 1, Arrays.asList("a", "b")),
arguments("lemon", 2, Arrays.asList("x", "y"))
);
}
| @@ -197,6 +200,8 @@ private StandardDestinationDefinition destinationDefinitionFromCreate(final Dest | |||
| destinationDefCreate.getDockerRepository(), | |||
| destinationDefCreate.getDockerImageTag()); | |||
|
|
|||
| final AirbyteVersion airbyteProtocolVersion = AirbyteProtocolVersion.getWithDefault(spec.getProtocolVersion()); | |||
There was a problem hiding this comment.
Not directly related to this PR but AirbyteVersion is confusing. Should we rename it SemanticVersion because it might not be an AirbyteVersion but a protocolVersion instead.
There was a problem hiding this comment.
Agree, I was thinking of using a 3rd party Version implementation (I saw maven/jackson) until I found out that we had this one already. The slight difference is that our implementation allows the version to be dev.
Happy to submit a PR follow up PR to rename it.
* Update protocol version from actor defs API operations * Implement default airbyte protocol version support * Add version parsing * Add acceptance tests * Fix Acceptance Tests * format * Make test package private
* Update protocol version from actor defs API operations * Implement default airbyte protocol version support * Add version parsing * Add acceptance tests * Fix Acceptance Tests * format * Make test package private
What
Store the protocol version of the connector when creating/updating a specific connector.
Closes #15978