add test that migration output schema same as source schema #3356

cgardens · 2021-05-11T03:20:06Z

closes #1862

What

Fix issue in migrations where output schemas were only using part of the schema. The problem is that the way the schemas were being pulled in was not following references. Now they do.
Add a test that checks that the configs in airbyte-config and airbyte-db match the configs in the output schema of the last migration
Going to stop iterating here. I think this hits the main case that's been biting us. We can add more coverage for other cases as they arise. I describe below what is and is not tested.

What this will catch

configs that are removed to airbyte-config & airbyte-db but not added to the migrations schema.
configs that are edited in airbyte-config & airbyte-db but not in the migrations schema. (i think this is the most common case)

What this will NOT catch

~~changes to the postgres database schema.~~ (Update: Added dbs to this PR)
configs that are added to airbyte-config & airbyte-db but not added to the migrations schema.
when the schema.sql is updated but the json schema objects that define it are not updated.

davinchia · 2021-05-12T05:51:21Z

airbyte-migration/src/main/java/io/airbyte/migrate/migrations/MigrationV0_23_0.java

+import org.slf4j.LoggerFactory;
+
+/**
+ * This migration is currently empty and is a placeholder for migrations for the next 0.19.0 release


nit: update this

davinchia · 2021-05-12T07:38:41Z

Looks good!

Possible dumb question since I'm not super familiar with what is persisted yet. I see a lot more models. Are StandardSourceDefinition, StandardDestinationDefinition and StandardSource the only ones added in this PR because they are the only models persisted across versions?

I agree on configs that are added to airbyte-config & airbyte-db but not added to the migrations schema. not being as important since this is additive and unlikely to break backward compatibility.

when the schema.sql is updated but the json schema objects that define it are not updated. seems more important to me since it it more likely to break backward compatibility. Can we create an issue to track this so we don't forget?

None of this is blocking since this is much better than what we currently have.

cgardens · 2021-05-12T20:54:25Z

the only ones added in this PR because they are the only models persisted across versions?

@davin good question. They are added in the PR because they are actually out of date. We forgot to run a migration :D. Not a big deal as the changes are backwards compatible additive changes. Never the less I want to get the record straight.

when the schema.sql is updated but the json schema objects that define it are not updated. seems more important to me since it it more likely to break backward compatibility. Can we create an issue to track this so we don't forget?

yeah. i'll create the issue. tbh, i'm going to wait for this to cause a bug before i think it'll be worth doing. but having the issue to track it is a good point. the test we could write here is to run a converter of .sql files into jsonschema and then confirm that the json schema in airbyte-db matches that which is converted from the sql file. or we could even drop the hardcoded jsonschema files and just generate the jsonschema files--would have to play with it to know what the right thing is.

davinchia · 2021-05-13T02:49:18Z

that sounds great as an issue description to begin with.

agree with you on waiting till this bites us to do this. thanks!

cgardens · 2021-05-14T00:49:16Z

the described issue: https://github.com/airbytehq/airbyte-internal-issues/issues/89

@avaidyanatha

* Requirements updated to CDK. airbyte-protocol and base-python requirements removed. * Bugfix: BufferedStreamConsumer. (#3387) * Format. * Bump versions. * main_dev.py renamed to main.py README.md updated * Source Stripe: Add Acceptance Tests to Stripe Connector (#3367) * Add Acceptance Tests to Stripe Connector * move configured_catalog.json to sample_files * bump version Co-authored-by: ykurochkin <y.kurochkin@zazmic.com> * Legacy lib references removed * FB Marketing source - lookback window logic not functioning correctly * FB Marketing source #1390 - returning buffered record while incremental sync * FB Marketing source #1390 - improving checking while syncing buffered record * FB Marketing source #1390 - adding loop_back to IncrementalStreamAPI * FB Marketing source #1390 - bump version * FB Marketing source #1390 - add CHANGELOG.md * Stop formatting python with spotless (#3388) * add test that migration output schema same as source schema (#3356) * Add updated architecture diagram to high level docs. (#3399) * Add updated architecture doc to high level docs. * Address review comments Co-authored-by: Abhi Vaidyanatha <abhivaidyanatha@Abhis-MacBook-Pro.local> * Correct GA readme error. (#3407) * make shopify more resilient to timeouts (#3409) * Update migration schema to include recent changes to the StandardSync object. (#3414) * Update all of Pydantic to 1.6.2 per Dependabot. (#3408) * Update all to 1.6.2. * Publish new airbyte-cdk version. * Use repr instead of str for exceptions. * Use rc. * Edit test. * Bump for SAT. * Format. * Docker ignore update. Fix setup.py * fixing ONLY problematic fields in freshdesk JSON schemas (#3376) * bump airbyte-webapp version (#2266) * add configuration for bumping webapp versionn * set to current version * Bump version: 0.16.0-alpha → 0.16.1-alpha * Revert "Bump version: 0.16.0-alpha → 0.16.1-alpha" Thiss reverts commit fdbf6dc. * also update package lock so we don't run into files changed errors * use 0.19.0-alpha * add npm webapp version * Add a CDK speedrun tutorial doc (#3403) * Add CDK Speedrun document. * Finish speedrun doc. * Address review comments * Add to SUMMARY.md Co-authored-by: Abhi Vaidyanatha <abhivaidyanatha@Abhis-MacBook-Pro.local> * Add Rust as a connector specific dependency to source-file (#3426) * Add Rust as a connector specific dependency to source-file * Add more details about installation. * Markdown lines are weird. Co-authored-by: Abhi Vaidyanatha <abhivaidyanatha@Abhis-MacBook-Pro.local> * API update to latest airbyte-cdk version * Add section Deploy Local on Windows (#3425) * add deploy on windows steps * correct minor * change suggestions by @avaidyanatha * GitBook: [master] 161 pages and 75 assets modified * Display icons (#3140) * Display icons * Improve icons views * MS SQL Server Destination implementation Fixes issue #613. Normalization is not yet enabled. This will have to be added at a later point. * Workflow to handle operations (custom transformation) (#3379) * Keep normalization backward compatible with old settings from destination * Bumpversion normalization image * add npm install before all npm run generates' (#3442) * restart containers if they fail automatically (#3423) * Update link for contribution scheduling (#3443) * Address issue with icon in onboarding (#3437) * rename toy connector tutorial to "Build a connector the hard way" (#3421) * Upload test reports (from integration test slash commands) as GitHub artifacts (#3416) * Archive test reports in github workflow * Archive Test reports only when failures * Fixing SqlServerOperations.java (#3454) Fixing some issues with `SqlServerOperations`, which was out of sync with recent changes to `SqlOperations`. * Add redirect to cdk tutorial page (#3456) * add redirect to cdk tutorial page * change path to cdk README.md Co-authored-by: Davin Chia <davinchia@gmail.com> Co-authored-by: Yevhenii <34103125+yevhenii-ldv@users.noreply.github.com> Co-authored-by: ykurochkin <y.kurochkin@zazmic.com> Co-authored-by: vitaliizazmic <75620293+vitaliizazmic@users.noreply.github.com> Co-authored-by: Charles <giardina.charles@gmail.com> Co-authored-by: Abhi Vaidyanatha <abhi@airbyte.io> Co-authored-by: Abhi Vaidyanatha <abhivaidyanatha@Abhis-MacBook-Pro.local> Co-authored-by: Jared Rhizor <jared@dataline.io> Co-authored-by: vovavovavovavova <39351371+vovavovavovavova@users.noreply.github.com> Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com> Co-authored-by: Marcos Marx <marcos@airbyte.io> Co-authored-by: Artem Astapenko <3767150+Jamakase@users.noreply.github.com> Co-authored-by: masonwheeler <masonwheeler@yahoo.com> Co-authored-by: Christophe Duong <christophe.duong@gmail.com> Co-authored-by: Sherif A. Nada <snadalive@gmail.com> Co-authored-by: Michel Tricot <michel@dataline.io>

tuliren · 2021-05-20T18:20:38Z

They are added in the PR because they are actually out of date. We forgot to run a migration :D. Not a big deal as the changes are backwards compatible additive changes. Never the less I want to get the record straight.

I think the three updated schemas should be added to migration v0.20.0, instead of v0.23.0, because they existed in that earlier version. During migration, the schema validator checks the configs for each version. So when the v0.20.0 production configs already contain the icon and prefix fields, the validator throws an exception, even though those fields are specified in the v0.23.0 migration. A fix is in #3503.

auto-assign bot requested review from jrhizor and sherifnada May 11, 2021 03:20

cgardens requested a review from davinchia May 11, 2021 03:20

cgardens added 3 commits May 11, 2021 16:23

add test that migration output schema same as source schema

80cb61c

remove print

2a9e16e

add jobs test

69e9da7

cgardens force-pushed the cgardens/add_test_for_configs branch from 4b819ba to 69e9da7 Compare May 11, 2021 23:23

cgardens requested a review from ChristopheDuong May 12, 2021 00:43

davinchia reviewed May 12, 2021

View reviewed changes

davinchia approved these changes May 12, 2021

View reviewed changes

cgardens merged commit 9b3b692 into master May 14, 2021

cgardens deleted the cgardens/add_test_for_configs branch May 14, 2021 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add test that migration output schema same as source schema #3356

add test that migration output schema same as source schema #3356

cgardens commented May 11, 2021 •

edited

Loading

davinchia May 12, 2021

davinchia commented May 12, 2021 •

edited by tuliren

Loading

cgardens commented May 12, 2021 •

edited

Loading

davinchia commented May 13, 2021

cgardens commented May 14, 2021

tuliren commented May 20, 2021

add test that migration output schema same as source schema #3356

add test that migration output schema same as source schema #3356

Conversation

cgardens commented May 11, 2021 • edited Loading

What

What this will catch

What this will NOT catch

davinchia May 12, 2021

Choose a reason for hiding this comment

davinchia commented May 12, 2021 • edited by tuliren Loading

cgardens commented May 12, 2021 • edited Loading

davinchia commented May 13, 2021

cgardens commented May 14, 2021

tuliren commented May 20, 2021

cgardens commented May 11, 2021 •

edited

Loading

davinchia commented May 12, 2021 •

edited by tuliren

Loading

cgardens commented May 12, 2021 •

edited

Loading