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

Add connection migrations for schema changes #17651

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

alovew
Copy link
Contributor

@alovew alovew commented Oct 6, 2022

Add three new columns to support the auto-detect schema change feature:

  • non_breaking_schema_change_preference ("ignore", "disable" enum)
  • breaking_change (boolean)
  • notify_schema_change (boolean)

@alovew alovew temporarily deployed to more-secrets October 6, 2022 00:12 Inactive
Copy link
Contributor

@gosusnp gosusnp left a comment

Choose a reason for hiding this comment

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

Looks good, mostly about the naming of the columns, I feel they are a bit generic to be added to connections.

ctx.alterTable("connection")
.addColumnIfNotExists(DSL.field(
"notify_schema_changes",
SQLDataType.BOOLEAN.nullable(false).defaultValue(true)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we want to have it on by default, just to make sure do we want to turn it on automatically for all existing connections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is yes, but I can double check with Andy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

he confirms yes

private static void addBreakingChange(final DSLContext ctx) {
ctx.alterTable("connection")
.addColumnIfNotExists(DSL.field(
"breaking_change",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is about notifications, can we be more explicit and add it to the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not about notifications - this is a boolean about whether this connection has breaking changes

private static void addNonBreakingChangePreference(final DSLContext ctx) {
ctx.alterTable("connection")
.addColumnIfNotExists(DSL.field(
"non_breaking_change_preference",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is about notifications, can we be more explicit and add it to the name?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for @gosusnp's suggestion. If this is for notification, we should probably have that in the column name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is also not about notifications - this is about the user's preference for how we handle non-breaking changes: either disable the connection or ignore

@alovew alovew requested a review from gosusnp October 6, 2022 17:13
@@ -138,7 +138,7 @@ void testBootloaderAppBlankDb() throws Exception {
val configsMigrator = new ConfigsDatabaseMigrator(configDatabase, configsFlyway);
// this line should change with every new migration
// to show that you meant to make a new migration to the prod database
assertEquals("0.40.11.001", configsMigrator.getLatestMigration().getVersion().getVersion());
assertEquals("0.40.11.002", configsMigrator.getLatestMigration().getVersion().getVersion());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters but it should be 0.40.13.000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there was an upgrade since Wednesday when I wrote this

@alovew alovew merged commit b22a439 into master Oct 7, 2022
@alovew alovew deleted the anne/connection-migration branch October 7, 2022 20:28
letiescanciano added a commit that referenced this pull request Oct 10, 2022
…vation

* master: (22 commits)
  Update full-refresh-append.md (#17784)
  Update full-refresh-overwrite.md (#17783)
  Update incremental-append.md (#17785)
  Update incremental-deduped-history.md (#17786)
  Update cdc.md (#17787)
  🪟 🔧 Ignore classnames during jest snapshot comparison (#17773)
  feat: replace openjdk with amazoncorretto:17.0.4 on connectors for seсurity compliance (#17511)
  Start testing buildpulse. (#17712)
  Add missing types to the registry (#17763)
  jobs db descriptions (#16543)
  config db data catalog (#16427)
  Update lowcode docs (#17752)
  db migrations to support new webhook operations (#17671)
  Bump Airbyte version from 0.40.13 to 0.40.14 (#17762)
  September Release Notes (#17754)
  Revert "Use java-datadog-tracer-base image (#17625)" (#17759)
  Add connection migrations for schema changes (#17651)
  Connection Form Refactor - Part Two (#16748)
  Improve E2E testing around the Connection Form (#17577)
  Bump strict encrypt version (#17747)
  ...
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants