-
Notifications
You must be signed in to change notification settings - Fork 194
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
Support gormio #825
Support gormio #825
Conversation
💚 CLA has been signed |
@Deepak13245 thanks for the PR! Looks pretty good, but I'm wondering if the callbacks are still necessary. If context is now first class, then perhaps we can instead just rely on Also, I'd like to call this module |
@axw renamed to apmgormv2 Thanks |
A couple of things bother me about the plugin approach:
What do you think about providing "Open" functions inside the dialect packages instead? So instead of import (
"gorm.io/driver/sqlite"
"gorm.io/gorm"
)
db, err := gorm.Open(sqlite.Open("gorm.db"), &gorm.Config{}) you could write import (
"go.elastic.co/apm/module/apmgormv2/dialect/apmsqlite"
"gorm.io/gorm"
)
db, err := gorm.Open(apmsqlite.Open("gorm.db"), &gorm.Config{}) Internally this would use the recently introduced support for custom drivers: go-gorm/sqlite#11 Basically just: package apmsqlite // go.elastic.co/module/apmgormv2/dialect/apmsqlite
import (
"gorm.io/driver/sqlite"
"gorm.io/gorm"
"go.elastic.co/module/apm/module/apmsql"
_ "go.elastic.co/module/apm/module/apmsql/sqlite3"
)
func Open(dsn string) gorm.Dialector {
return &sqlite.Dialector{
DriverName: apmsql.DriverPrefix + sqlite.DriverName,
DSN: dsn,
}
} |
@axw creating dialect from with driver prefix makes it neat, updated the pr please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I agree this looks much neater.
jenkins run the tests please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks!
jenkins run the tests please |
It looks like gorm v2 requires Go 1.13+, so we'll need to update the build constraint. |
Looks good, I think the remaining test failures are fixed on master. Can you please merge master so we can make sure? |
10999ee
to
29ffa5d
Compare
CI looks happy. Thanks for another nice PR, and for bearing with my requests :) |
Resolves #810