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

Change the Generate function to take a SchemaSource instead of a sqldb.Queryable #189

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

the-glu
Copy link
Contributor

@the-glu the-glu commented Jan 28, 2025

Description

This PR change the Generate function to take a SchemaSource instead of a sqldb.Queryable.

Motivation

We're using this library to generate migrations and this option allows us to generate forward and backward migrations, based on the wanted schema and database.

The change allows for better flexibility in Generate and let's us just swap arguments to the function, to generate both migrations at once.

Testing

Tests with the pg-schema-diff-test-runner are passing locally :)

Copy link

cla-assistant bot commented Jan 28, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Jan 28, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

Thanks for the contribution! I fundamentally agree with what you're trying to do...but maybe think WithReverse is a bit confusing and indicates a bigger flaw with the pg-schema-diff API.

Depending on how much work you want to do here:

With code change

Replace sqldb.Queryable with SchemaSource and change fromDB to fromSchema. This will break backwards compatibility, but I think that's okay(?)

No Code Change

  • Use the tempdbfactory to create a database with your target schema DDL and pass that as sqldb.Queryable
  • Use the database schema source as the target schema

Let me know if you're interested in the more in-depth code change! I could also try to throw this together!

@the-glu
Copy link
Contributor Author

the-glu commented Jan 29, 2025

Thanks for having a look!

Let me know if you're interested in the more in-depth code change! I could also try to throw this together!

Ok, I can also go with the code change if you're OK with breaking backwards compatibility :)

Any specific hints or direction for the change? I see there is already a DBSchemaSource, so it should be doable without big modifications.

@the-glu the-glu changed the title Add option to generate a reversed diff Change the Generate function to take a SchemaSource instead of a sqldb.Queryable Feb 3, 2025
@the-glu
Copy link
Contributor Author

the-glu commented Feb 3, 2025

The PR has been updated with the new code change :)

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.

Looks great! Could you update the code example in the README.md?

@the-glu
Copy link
Contributor Author

the-glu commented Feb 4, 2025

Looks great! Could you update the code example in the README.md?

Yes, it should be fixed now :)

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.

Thanks for updating the README.md! Changes look good to me. Appreciate the contribution!

@bplunkett-stripe bplunkett-stripe merged commit 7e225f6 into stripe:main Feb 4, 2025
8 checks passed
@the-glu the-glu deleted the reverse branch February 4, 2025 17:25
@the-glu
Copy link
Contributor Author

the-glu commented Feb 4, 2025

Thanks you for the quick review and the approval :)

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