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 support for multiple pragma statements and driver syntax #3031

Merged
merged 3 commits into from
Jun 30, 2022
Merged

add support for multiple pragma statements and driver syntax #3031

merged 3 commits into from
Jun 30, 2022

Conversation

jbreiding
Copy link
Contributor

@jbreiding jbreiding commented Jun 28, 2022

What changed?
Change in how SQLite dsn is constructed to match upstream driver

Why?
Support multiple pragma statements
Change in upstream driver for file uri

How did you test it?
Updated unit tests
Local dev environment

Potential risks
None

Is hotfix candidate?
Yes

@jbreiding jbreiding requested a review from a team as a code owner June 28, 2022 18:06
@robholland
Copy link
Contributor

Makes sense to me. This is not backward compatible though, previous style pragma settings will be silently ignored. Maybe we should check for the previous pragma style settings and throw an error asking them to update their config for a couple of versions?

@jbreiding
Copy link
Contributor Author

jbreiding commented Jun 28, 2022

Makes sense to me. This is not backward compatible though, previous style pragma settings will be silently ignored. Maybe we should check for the previous pragma style settings and throw an error asking them to update their config for a couple of versions?

Instead of erroring, we can be backwards compatible.

Copy link
Contributor

@robholland robholland left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jbreiding jbreiding enabled auto-merge (squash) June 28, 2022 20:45
@jbreiding jbreiding disabled auto-merge June 28, 2022 22:34
@jbreiding jbreiding enabled auto-merge (squash) June 29, 2022 01:25
Co-authored-by: Alex Shtin <alex@shtin.com>
Co-authored-by: Alex Shtin <alex@shtin.com>
@jbreiding jbreiding disabled auto-merge June 30, 2022 18:46
@jbreiding jbreiding merged commit c7866bb into temporalio:master Jun 30, 2022
@jbreiding jbreiding deleted the modernc-pragram-support branch June 30, 2022 19:56
jbreiding added a commit that referenced this pull request Jun 30, 2022
* add support for multiple pragma statements and driver syntax

* Update common/persistence/sql/sqlplugin/sqlite/driver.go

Co-authored-by: Alex Shtin <alex@shtin.com>

* Update common/persistence/sql/sqlplugin/sqlite/plugin.go

Co-authored-by: Alex Shtin <alex@shtin.com>

Co-authored-by: Alex Shtin <alex@shtin.com>
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.

4 participants