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

feat: Support SQLAlchemy 2.0 #185

Merged
merged 18 commits into from
Oct 20, 2023
Merged

feat: Support SQLAlchemy 2.0 #185

merged 18 commits into from
Oct 20, 2023

Conversation

andyoneal
Copy link
Contributor

Most of the changes had to do with SQLAlchemy no longer defaulting to auto-commit. You have to either use a begin context or explicitly commit. Also a handful of changes on when an engine/connection is bound to metadata/reflect.

@visch
Copy link
Member

visch commented Sep 8, 2023

@andyoneal tests looks to be failing here, can we move this to Draft until it's ready?

@andyoneal
Copy link
Contributor Author

andyoneal commented Sep 13, 2023 •
edited
Loading

@visch sorry for not noticing the errors. I think this is a bug in the sdk. I've opened a PR there, but implemented the fix here for now. meltano/sdk#1954

@edgarrmondragon edgarrmondragon self-requested a review September 22, 2023 15:39
@visch visch mentioned this pull request Sep 23, 2023
Closed
edgarrmondragon
edgarrmondragon approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

This LGTM!

@andyoneal
Copy link
Contributor Author

just a note for later: the "fix create_schema" commit can be reverted once this moves to SDK 0.23

@edgarrmondragon
Copy link
Member

just a note for later: the "fix create_schema" commit can be reverted once this moves to SDK 0.23

@andyoneal I just merged #199

edgarrmondragon
edgarrmondragon reviewed Oct 3, 2023
View reviewed changes
Copy link
Member

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

The tap side of the code LGTM, but I'm not as familiar with what the tests are originally doing.

@edgarrmondragon
Copy link
Member

Thanks for the additional context on test file changes @andyoneal! If Derek doesn't have any objections, I'm happy to merge and make a v0.1.0b1 release next week.

@visch
Copy link
Member

visch commented Oct 7, 2023

Thanks for the additional context on test file changes @andyoneal! If Derek doesn't have any objections, I'm happy to merge and make a v0.1.0b1 release next week.

Generally it looks good I just haven't made the time to dive into the specifics of connection.begin and how / where we are doing those transactions

visch
visch approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@visch visch left a comment

Choose a reason for hiding this comment

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

Great PR!

@visch visch merged commit 1468179 into MeltanoLabs:main Oct 20, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgarrmondragon edgarrmondragon edgarrmondragon approved these changes

@visch visch visch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andyoneal @visch @edgarrmondragon