-
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
Fix for acceptance test on discover workflow #22595
Changes from 7 commits
1bf8a6b
70f476f
c430aba
274d481
4741924
1a8289b
83a53c5
2a42f49
3688fd8
2f17718
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ | |
import io.airbyte.workers.general.DefaultCheckConnectionWorker; | ||
import io.airbyte.workers.general.DefaultDiscoverCatalogWorker; | ||
import io.airbyte.workers.general.DefaultGetSpecWorker; | ||
import io.airbyte.workers.helper.CatalogClientConverters; | ||
import io.airbyte.workers.helper.ConnectorConfigUpdater; | ||
import io.airbyte.workers.helper.EntrypointEnvChecker; | ||
import io.airbyte.workers.internal.AirbyteSource; | ||
|
@@ -125,11 +126,9 @@ public abstract class AbstractSourceConnectorTest { | |
private ConnectorConfigUpdater mConnectorConfigUpdater; | ||
|
||
// This has to be using the protocol version of the platform in order to capture the arg | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this comment makes sense to keep anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that meant to be AirbyteCatalog as it has to be using protocol version, but yeah definitely not used to capture the arg |
||
private final ArgumentCaptor<io.airbyte.protocol.models.AirbyteCatalog> lastPersistedCatalog = | ||
ArgumentCaptor.forClass(io.airbyte.protocol.models.AirbyteCatalog.class); | ||
|
||
protected AirbyteCatalog getLastPersistedCatalog() { | ||
return convertProtocolObject(lastPersistedCatalog.getValue(), AirbyteCatalog.class); | ||
return convertProtocolObject( | ||
CatalogClientConverters.toAirbyteProtocol(discoverWriteRequest.getValue().getCatalog()), AirbyteCatalog.class); | ||
} | ||
|
||
private final ArgumentCaptor<SourceDiscoverSchemaWriteRequestBody> discoverWriteRequest = | ||
|
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.
It definitely makes me feel uneasy to duplicate such complicated and hard-to-maintain code. It seems like it will be very likely that this code will need to change in both places, but it will only end up being changed in a single place because it isn't obvious that there are multiple implementations of nearly the exact same thing.
I don't really have a good answer for this right now, since the conversion logic isn't actually duplicated (in one case, they are server-side java models, and here they are client-side java models, so they're technically different types).
I think it would be ideal to add very apparent comments to this class AND to the other CatalogConverter class that the logic is duplicated for now, and we should discuss and try to prioritize an issue for coming up with a better approach for these super-complicated catalog conversion methods.
I don't want to block this PR, but I want to make sure we're doing everything we can to minimize the tech debt here and come up with a solid long term solution!
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.
I agree - conversion between those three types with added logic for stream makes it really hard to maintain.