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 basic Extension support. #43

Merged

Conversation

alexrhee-stripe
Copy link
Collaborator

@alexrhee-stripe alexrhee-stripe commented Jul 13, 2023

Description

We add basic extension support. This will allow declarative DDL sources to instruct the tooling to create, alter, and delete extensions so that other DDL statements dependent on these extensions are executable.

Note that the full gamut of extension DDL is not currently supported, namely the ability to target and move extensions across schemas. Insofar as the schema migration tooling assumes everything is on the public schema, I think this is OK for now and can be implemented in a later PR.

Motivation

We need extension support.

Testing

Unit tests were added and current tests were augmented. As a result of this change, we removed the onDbInitQueries hook since extensions are actually supported and would otherwise require adjusting every single test case. In its stead, I altered the internal/migration_acceptance_tests/function_cases_test.go to include a case with an extension known to create functions and we observe they are correctly ignored.

@alexrhee-stripe alexrhee-stripe changed the title Alexrhee/extension support Add basic Extension support. Jul 13, 2023
@alexrhee-stripe alexrhee-stripe marked this pull request as ready for review July 13, 2023 18:52
Copy link
Collaborator

@bplunkett-stripe bplunkett-stripe left a comment

Choose a reason for hiding this comment

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

I'm excited to get this in! A couple suggested changes but nothing major.

Also be sure to add a note to the README saying we support this! 🥳

internal/queries/queries.sql Outdated Show resolved Hide resolved
internal/schema/schema.go Outdated Show resolved Hide resolved
pkg/diff/schema_migration_plan_test.go Outdated Show resolved Hide resolved
pkg/diff/sql_generator.go Outdated Show resolved Hide resolved
pkg/diff/sql_generator.go Outdated Show resolved Hide resolved
internal/schema/schema.go Show resolved Hide resolved
pkg/diff/plan.go Outdated Show resolved Hide resolved
pkg/diff/sql_generator.go Outdated Show resolved Hide resolved
pkg/diff/sql_generator.go Outdated Show resolved Hide resolved
@alexrhee-stripe
Copy link
Collaborator Author

@bplunkett-stripe , thanks for the feedback! I think this is ready for another look.

Copy link
Collaborator

@bplunkett-stripe bplunkett-stripe left a comment

Choose a reason for hiding this comment

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

LGTM with one small nit

@@ -28,6 +28,7 @@ pg-schema-diff plan --dsn "postgres://postgres:postgres@localhost:5432/postgres"
- Partitions
- Functions/Triggers (functions created by extensions are ignored)
- Sequences
- Extensions
Copy link
Collaborator

Choose a reason for hiding this comment

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

🥳

pkg/diff/sql_generator.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@bplunkett-stripe bplunkett-stripe left a comment

Choose a reason for hiding this comment

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

🥳 Excited to get this in!

@alexrhee-stripe alexrhee-stripe merged commit b3b6eaa into stripe:main Jul 17, 2023
5 checks passed
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.

2 participants