Skip to content
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

source-mssql: convert to bulk CDK #48465

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stephane-airbyte
Copy link
Contributor

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

  • YES πŸ’š
  • NO ❌

Copy link

vercel bot commented Nov 12, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 9:01pm

@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Nov 12, 2024
Copy link
Contributor Author

stephane-airbyte commented Nov 12, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@stephane-airbyte stephane-airbyte force-pushed the stephane/11-12-source-mssql_convert_to_bulk_cdk branch 4 times, most recently from ff6891f to 5a4d483 Compare November 12, 2024 22:38
@stephane-airbyte stephane-airbyte force-pushed the stephane/11-12-source-mssql_convert_to_bulk_cdk branch 2 times, most recently from a63db79 to 8bb623d Compare November 13, 2024 18:55
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superficial review LGTM, this is a good base to build on!

@@ -38,19 +38,6 @@ data:
message: "Add default cursor for cdc"
upgradeDeadline: "2023-08-23"
connectorTestSuitesOptions:
- suite: unitTests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the removal of integrationTests but is removing unitTests intentional?

import jakarta.inject.Singleton

/**
* The object which is mapped to the Mysql source configuration JSON.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do a string search&replace for /mysql/mssql/ in this source tree before we forget to do so :)

* Copyright (c) 2024 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.integrations.source.mssql.config_spec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently underscores in package names makes some people sad

@JsonSchemaInject(json = """{"order":8, "default":"CDC", "display_type": "radio"}""")
fun getReplicationMethodValue(): MsSqlServerReplicationMethodConfiguration? =
replicationMethodJson ?: replicationMethod.asReplicationMethod()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed over this and the rest of the package but it seems legit. If your unit tests are happy then I'm happy.

@@ -321,7 +321,7 @@ data object UserDefinedCursor : CursorMethodConfiguration
@JsonSchemaDescription(
"<i>Recommended</i> - " +
"Incrementally reads new inserts, updates, and deletes using Mysql's <a href=" +
"\"https://docs.airbyte.com/integrations/sources/mssql/#change-data-capture-cdc\"" +
"\"https://docs.airbyte.com/integrations/sources/mysql/#change-data-capture-cdc\"" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catches! should these changes be in a separate PR? probably, because you'll have to bump the version of source-mysql

},
"additionalProperties": true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this isn't too different from the legacy spec?

@stephane-airbyte stephane-airbyte force-pushed the stephane/11-12-source-mssql_convert_to_bulk_cdk branch 13 times, most recently from dd1bac8 to 2b752b8 Compare November 18, 2024 19:32
@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Nov 18, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/11-12-source-mssql_convert_to_bulk_cdk branch 4 times, most recently from 0df7cbd to c3296db Compare November 19, 2024 03:29
@stephane-airbyte stephane-airbyte force-pushed the stephane/11-12-source-mssql_convert_to_bulk_cdk branch 13 times, most recently from 1fdf785 to 4c34884 Compare November 22, 2024 17:51
@stephane-airbyte stephane-airbyte force-pushed the stephane/11-12-source-mssql_convert_to_bulk_cdk branch 2 times, most recently from 067100e to 1058bcc Compare December 4, 2024 20:06
@octavia-squidington-iii octavia-squidington-iii removed the CDK Connector Development Kit label Dec 4, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/11-12-source-mssql_convert_to_bulk_cdk branch 2 times, most recently from 070288f to b685ade Compare December 5, 2024 21:14
@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Dec 5, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/11-12-source-mssql_convert_to_bulk_cdk branch from b685ade to f9e0271 Compare December 11, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants