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: aligned regular and cdc syncs and its datatype tests #14379

Merged

Conversation

etsybaev
Copy link
Contributor

@etsybaev etsybaev commented Jul 3, 2022

What

Currently, we have 2 different classes for integration datatype tests CdcMssqlSourceDatatypeTest and MssqlSourceDatatypeTest. They have the same input args, but differ "expected result" for some types.
Some of Cdc types were not supported at all.

How

Aligned regular and cdc syncs and its datatype tests + added support for some missed new types in CDC migration.

Recommended reading order

  1. airbyte-integrations/connectors/source-mssql/src/test-integration/java/io/airbyte/integrations/source/mssql/AbstractMssqlSourceDatatypeTest.java
  2. airbyte-integrations/connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSourceOperations.java
  3. airbyte-integrations/bases/debezium-v1-4-2/src/main/java/io/airbyte/integrations/debezium/internals/MSSQLConverter.java

🚨 User Impact 🚨

Shouldn't introduce any breaking changes.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here
Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here
Connector Generator
  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

@github-actions github-actions bot added the area/connectors Connector related issues label Jul 3, 2022
@etsybaev etsybaev temporarily deployed to more-secrets July 3, 2022 13:19 Inactive
@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 4, 2022

/test connector=connectors/source-mssql

🕑 connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/2608386871
✅ connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/2608386871
No Python unittests run

Build Passed

Test summary info:

All Passed

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jul 5, 2022
@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 5, 2022

@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 5, 2022

/test connector=connectors/source-mssql

🕑 connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/2616751722
✅ connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/2616751722
No Python unittests run

Build Passed

Test summary info:

All Passed

@etsybaev etsybaev temporarily deployed to more-secrets July 5, 2022 14:13 Inactive
@etsybaev etsybaev changed the title [13606] source-mssql: aligned regular and cdc syncs and its datatype … 🐛Source-mssql: aligned regular and cdc syncs and its datatype tests + added support for new types Jul 5, 2022
@etsybaev etsybaev marked this pull request as ready for review July 5, 2022 14:23
@etsybaev etsybaev temporarily deployed to more-secrets July 5, 2022 14:29 Inactive
@etsybaev etsybaev requested a review from grishick July 5, 2022 14:47
@etsybaev etsybaev temporarily deployed to more-secrets July 5, 2022 21:05 Inactive
@grishick
Copy link
Contributor

grishick commented Jul 6, 2022

@etsybaev could you please separate functional changes that add support for new types from the refactoring of tests and split this into two PRs:

  1. add support for some missed new types in CDC migration
  2. test refactoring

@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 6, 2022

@etsybaev could you please separate functional changes that add support for new types from the refactoring of tests and split this into two PRs:

  1. add support for some missed new types in CDC migration
  2. test refactoring

hi @grishick .
A lot of tests will start failing if I remove the logic to support new datatypes (that never worked before for CDC). Because now we have the same tests for both normal and CDC migrations. I had to add it to make tests passed on both migrations types.
It would also take me lot's of additional time. As I firstly would need to revert all the changes, spend some time for fixing CDC tests. Wait for its merging, then again spend some time for merging both types into a single tests class and again wait for it to merge.
But if you insist I will do it.
Thanks

@etsybaev etsybaev requested a review from a team as a code owner July 14, 2022 18:36
@etsybaev etsybaev temporarily deployed to more-secrets July 14, 2022 18:38 Inactive
@etsybaev etsybaev changed the title 🐛Source-mssql: aligned regular and cdc syncs and its datatype tests + added support for new types 🐛Source-mssql: aligned regular and cdc syncs and its datatype tests Jul 14, 2022
@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 14, 2022

/test connector=connectors/source-mssql

🕑 connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/2672544357
✅ connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/2672544357
No Python unittests run

Build Passed

Test summary info:

All Passed

@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 14, 2022

/publish connector=connectors/source-mssql

🕑 Publishing the following connectors:
connectors/source-mssql
https://github.com/airbytehq/airbyte/actions/runs/2672979382


Connector Did it publish? Were definitions generated?
connectors/source-mssql

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@etsybaev etsybaev temporarily deployed to more-secrets July 14, 2022 20:34 Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets July 14, 2022 21:37 Inactive
@grishick
Copy link
Contributor

@etsybaev don't forget to update and publish source-mssql-strict-encrypt

@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 15, 2022

/test connector=source-mssql-strict-encrypt

🕑 source-mssql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/2675045483
✅ source-mssql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/2675045483
No Python unittests run

Build Passed

Test summary info:

All Passed

@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 15, 2022

/test connector=connectors/source-mssql-strict-encrypt

🕑 connectors/source-mssql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/2675160863
✅ connectors/source-mssql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/2675160863
No Python unittests run

Build Passed

Test summary info:

All Passed

@etsybaev etsybaev temporarily deployed to more-secrets July 15, 2022 06:53 Inactive
@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 15, 2022

/publish connector=connectors/source-mssql-strict-encrypt

🕑 Publishing the following connectors:
connectors/source-mssql-strict-encrypt
https://github.com/airbytehq/airbyte/actions/runs/2675226698


Connector Did it publish? Were definitions generated?
connectors/source-mssql-strict-encrypt

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@etsybaev
Copy link
Contributor Author

etsybaev commented Jul 15, 2022

/publish connector=connectors/source-mssql-strict-encrypt auto-bump-version=false

🕑 Publishing the following connectors:
connectors/source-mssql-strict-encrypt
https://github.com/airbytehq/airbyte/actions/runs/2676810126


Connector Did it publish? Were definitions generated?
connectors/source-mssql-strict-encrypt

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@etsybaev
Copy link
Contributor Author

As per logs from the second attempt the strict-encrypt image had been published from the first attempt

@etsybaev etsybaev merged commit 95b3424 into master Jul 15, 2022
@etsybaev etsybaev deleted the etsybaev/13606-source-mssql-aligned-regualr-and-cdc-syncs branch July 15, 2022 12:36
jsrcodes added a commit to jsrcodes/airbyte that referenced this pull request Jul 15, 2022
…rbytehq-master

* 'master' of https://github.com/airbytehq/airbyte: (1141 commits)
  pass USE_STREAM_CAPABLE_STATE env var to containers/deployments (airbytehq#14737)
  Bump mqtt connector (airbytehq#14648)
  Add error code to ManualOperationResult (airbytehq#14657)
  Bump elasticsearch version (airbytehq#14640)
  Ryan/sync oracle version number (airbytehq#14736)
  Fixed linter issue with add_fields.py comments (airbytehq#14742)
  🎉Redshift, Databricks, Snowflake, S3 Destinations: Make S3 output filename configurable (airbytehq#14494)
  🐛Source-mssql: aligned regular and cdc syncs and its datatype tests (airbytehq#14379)
  🎉 Source Amazon Seller Partner: Add new streams (airbytehq#13604)
  bump source-file-secure (airbytehq#14704)
  🎉 New source: Timely airbytehq#13292 (airbytehq#14335)
  🪟🔧 Refactor feature service (airbytehq#14559)
  [low code cdk] add a transformation for adding fields into an outgoing record (airbytehq#14638)
  Bump destination-postgres to 0.3.21 (airbytehq#14479)
  Remove `additionalProperties: false` from JDBC destination connectors (airbytehq#14618)
  🎉 Source Notion: add OAuth authorization for source-notion connector (airbytehq#14706)
  Use the configuration diff calculation in the update endpoint (airbytehq#14626)
  🪟 🐛 Fix input validation on blur and cleanup signup error handling (airbytehq#14724)
  lower sleep after wait for successful job (airbytehq#14725)
  Add configuration diff (airbytehq#14603)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source-mssql: align regular and CDC integration tests and data mappers
4 participants