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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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


val jobsPersistence = new DefaultJobPersistence(jobDatabase);
assertEquals(VERSION_0330_ALPHA, jobsPersistence.getVersion().get());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* Copyright (c) 2022 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.db.instance.configs.migrations;

import java.util.Arrays;
import org.flywaydb.core.api.migration.BaseJavaMigration;
import org.flywaydb.core.api.migration.Context;
import org.jooq.DSLContext;
import org.jooq.EnumType;
import org.jooq.impl.DSL;
import org.jooq.impl.SQLDataType;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class V0_40_11_002__AddSchemaChangeColumnsToConnections extends BaseJavaMigration {

private static final Logger LOGGER = LoggerFactory.getLogger(V0_40_11_002__AddSchemaChangeColumnsToConnections.class);

@Override
public void migrate(final Context context) throws Exception {
LOGGER.info("Running migration: {}", this.getClass().getSimpleName());

final DSLContext ctx = DSL.using(context.getConnection());

addNonBreakingChangePreferenceEnumTypes(ctx);

addNotifySchemaChanges(ctx);
addNonBreakingChangePreference(ctx);
addBreakingChange(ctx);
}

private static void addNonBreakingChangePreferenceEnumTypes(final DSLContext ctx) {
ctx.createType(NonBreakingChangePreferenceType.NAME)
.asEnum(Arrays.stream(NonBreakingChangePreferenceType.values()).map(NonBreakingChangePreferenceType::getLiteral).toList())
.execute();
}

private static void addNotifySchemaChanges(final DSLContext ctx) {
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

.execute();
}

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

SQLDataType.VARCHAR.asEnumDataType(NonBreakingChangePreferenceType.class).nullable(false)
.defaultValue(NonBreakingChangePreferenceType.IGNORE)))
.execute();

}

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

SQLDataType.BOOLEAN.nullable(false).defaultValue(false)))
.execute();
}

public enum NonBreakingChangePreferenceType implements EnumType {

IGNORE("ignore"),
DISABLE("disable");

private final String literal;
public static final String NAME = "non_breaking_change_preference_type";

NonBreakingChangePreferenceType(final String literal) {
this.literal = literal;
}

@Override
public String getName() {
return NAME;
}

@Override
public String getLiteral() {
return literal;
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ create table "public"."connection"(
"schedule_type" schedule_type null,
"schedule_data" jsonb null,
"geography" geography_type not null default null,
"notify_schema_changes" bool not null default true,
"non_breaking_change_preference" varchar(7) not null default '''ignore''::character varying',
"breaking_change" bool not null default false,
constraint "connection_pkey"
primary key ("id")
);
Expand Down