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.

Sign up for GitHub

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

Jump to bottom

Fix migration dev center schema dump by run db-specific initialization script #18984

Merged
merged 4 commits into from
Nov 4, 2022
Merged

Fix migration dev center schema dump by run db-specific initialization script #18984

merged 4 commits into from
Nov 4, 2022

Conversation

tuliren
Copy link
Contributor

@tuliren tuliren commented Nov 4, 2022 •
edited
Loading

What

  • Follow up for Ensure database initialization in test container #17697.
  • The migration dev center should not run initialization scripts for both databases. This will pollute the configs schema dump with tables from the jobs database, and vice versa.
  • The fix is to only run the initialization script for the specific db.
  • Add unit tests to ensure that this won't happen in the future.

@tuliren tuliren requested a review from jdpgrailsdev November 4, 2022 19:46
@tuliren tuliren changed the title Run db-specific initialization script Run db-specific initialization script to fix schema dump Nov 4, 2022
@tuliren tuliren mentioned this pull request Nov 4, 2022
Merged
@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 19:47 Inactive
@tuliren tuliren requested a review from terencecho November 4, 2022 19:50
@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 19:52 Inactive
@terencecho
Copy link
Contributor

@tuliren makes sense, thanks for putting out the fix! should we also revert the schema dump file changes in this PR?

@tuliren
Copy link
Contributor Author

tuliren commented Nov 4, 2022 •
edited
Loading

should we also revert the schema dump file changes in this PR?

The current schema dump files are correct in the codebase. This is because those dumps are eventually generated by the ConfigsDatabaseMigratorTest and JobsDatabaseMigratorTest, which initializes the database correctly.

This PR fixes the schema dumps generated by the dumpConfigsSchema and dumpJobsSchema gradle commands, which are only used locally when developing a migration.

@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 20:04 Inactive
terencecho
terencecho approved these changes Nov 4, 2022
View reviewed changes
Copy link
Contributor

@terencecho terencecho left a comment

Choose a reason for hiding this comment

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

lgtm!

@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 20:26 Inactive
@tuliren tuliren changed the title Run db-specific initialization script to fix schema dump Fix migration dev center schema dump by run db-specific initialization script Nov 4, 2022
@tuliren tuliren merged commit 104c91c into master Nov 4, 2022
@tuliren tuliren deleted the liren/fix-schema-dump branch November 4, 2022 20:28
letiescanciano added a commit that referenced this pull request Nov 7, 2022
@letiescanciano
Merge branch 'master' into leti/experiment-add-source-in-signup
c3edfb7 
* master: (69 commits)
  🪟 🐛 Fix wrong geography dropdown type #19021
  SAT: basic read on full catalog when `test_strictness_level == high` (#18937)
  Unhide DynamoDB destination (#18994)
  Fixed tests for destination connectors (#19007)
  🐛 Source Facebook Marketing: handle FacebookBadObjectError (#18971)
  Edit multi-cloud docs (#18972)
  🪟 🎉 Load credits consumption separate (#18986)
  Bmoric/extract source api (#18944)
  Migrating InvalidCursorException -> ConfigErrorException  (#18995)
  🪟 🎨 Fix banner link color (#18978)
  Handling configuration exceptions in IntegrationRunner (#18989)
  Add new workspace api endpoint (#18983)
  Add normalization to destination definition and actor definition table (#18300)
  Fix oauth controller (#18981)
  Fix migration dev center schema dump by run db-specific initialization script (#18984)
  fix master build failure (#18982)
  cleanup: delete debezium 1-4-2 module (#18733)
  Remove unused job persistence methods. (#18952)
  Hash filenames of extracted CSS (#18976)
  Fix typo in source code comment DataDaog ==> Datadog (#18911)
  ...
letiescanciano added a commit that referenced this pull request Nov 7, 2022
@letiescanciano
Merge branch 'master' into leti/move-oauth-to-the-top
6c7a97f 
* master: (73 commits)
  🪟 🐛 Fix wrong geography dropdown type #19021
  SAT: basic read on full catalog when `test_strictness_level == high` (#18937)
  Unhide DynamoDB destination (#18994)
  Fixed tests for destination connectors (#19007)
  🐛 Source Facebook Marketing: handle FacebookBadObjectError (#18971)
  Edit multi-cloud docs (#18972)
  🪟 🎉 Load credits consumption separate (#18986)
  Bmoric/extract source api (#18944)
  Migrating InvalidCursorException -> ConfigErrorException  (#18995)
  🪟 🎨 Fix banner link color (#18978)
  Handling configuration exceptions in IntegrationRunner (#18989)
  Add new workspace api endpoint (#18983)
  Add normalization to destination definition and actor definition table (#18300)
  Fix oauth controller (#18981)
  Fix migration dev center schema dump by run db-specific initialization script (#18984)
  fix master build failure (#18982)
  cleanup: delete debezium 1-4-2 module (#18733)
  Remove unused job persistence methods. (#18952)
  Hash filenames of extracted CSS (#18976)
  Fix typo in source code comment DataDaog ==> Datadog (#18911)
  ...
@github-actions github-actions bot mentioned this pull request Nov 15, 2022
Closed
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Nov 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terencecho terencecho terencecho approved these changes

@jdpgrailsdev jdpgrailsdev Awaiting requested review from jdpgrailsdev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tuliren @terencecho