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

Add script that prints clickhouse auto migration diff #3474

Merged
merged 13 commits into from
Mar 21, 2023
Merged

Add script that prints clickhouse auto migration diff #3474

merged 13 commits into from
Mar 21, 2023

Conversation

maggie-lou
Copy link
Contributor

@maggie-lou maggie-lou commented Feb 28, 2023 •
edited
Loading

Fixes buildbuddy-io/buildbuddy-internal#2096

@maggie-lou maggie-lou mentioned this pull request Feb 28, 2023
Merged
@maggie-lou
Copy link
Contributor Author

maggie-lou commented Feb 28, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

luluz66
luluz66 reviewed Mar 6, 2023
View reviewed changes
server/tables/tables.go Outdated Show resolved Hide resolved
server/tables/tables.go Outdated Show resolved Hide resolved
@maggie-lou maggie-lou mentioned this pull request Mar 7, 2023
Closed
@maggie-lou maggie-lou requested a review from luluz66 March 7, 2023 19:46
Base automatically changed from gorm_dump_migration to master March 7, 2023 20:32
luluz66
luluz66 reviewed Mar 7, 2023
View reviewed changes
server/tables/tables.go Outdated Show resolved Hide resolved
@maggie-lou maggie-lou changed the base branch from master to schema_diff_simp March 8, 2023 00:13
@maggie-lou maggie-lou mentioned this pull request Mar 8, 2023
Merged
@maggie-lou maggie-lou requested a review from luluz66 March 8, 2023 01:04
@maggie-lou maggie-lou force-pushed the clickhouse_print_migration branch 2 times, most recently from bd2cefb to b5e3e5e Compare March 8, 2023 02:32
Base automatically changed from schema_diff_simp to master March 8, 2023 16:03
@maggie-lou maggie-lou marked this pull request as draft March 8, 2023 17:09
@maggie-lou maggie-lou force-pushed the clickhouse_print_migration branch 2 times, most recently from 9034304 to 933085e Compare March 21, 2023 00:21
@maggie-lou maggie-lou marked this pull request as ready for review March 21, 2023 00:32
@maggie-lou
Copy link
Contributor Author

I realized I could simplify this script by fixing something in the gorm clickhouse driver. It's taking some time to get that change approved, so I added a local buildpatch in this PR. @luluz66 could you please take another look at this?

My PR to gorm-clickhouse with more context on the problem: go-gorm/clickhouse#91

As a reminder, currently clickhouse gorm autoMigrate always runs column migrations, even if there aren't any schema changes (the reason for that is describe in the PR above). In addition to being unnecessary, it also makes this schema change script less useful, because it appears as if we're always running a migration, and makes it harder to detect when we're making a meaningful schema change. Applying this patch will fix both of those problems

luluz66
luluz66 reviewed Mar 21, 2023
View reviewed changes
server/util/clickhouse/clickhouse.go Outdated Show resolved Hide resolved
server/util/gormutil/gormutil.go Outdated Show resolved Hide resolved
server/util/gormutil/gormutil.go Outdated Show resolved Hide resolved
@maggie-lou maggie-lou requested a review from luluz66 March 21, 2023 18:00
@maggie-lou maggie-lou merged commit 7d77351 into master Mar 21, 2023
@maggie-lou maggie-lou deleted the clickhouse_print_migration branch March 21, 2023 21:47
@maggie-lou
Copy link
Contributor Author

Fixes buildbuddy-io/buildbuddy-internal#2096

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luluz66 luluz66 luluz66 approved these changes

@tylerwilliams tylerwilliams Awaiting requested review from tylerwilliams

@bduffany bduffany Awaiting requested review from bduffany

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
@maggie-lou @luluz66