-
Notifications
You must be signed in to change notification settings - Fork 516
support/db: upgrade github.com/go-sql-driver/mysql from 2e00b5cd7039 to v1.4.0 #1652
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/db: upgrade github.com/go-sql-driver/mysql from 2e00b5cd7039 to v1.4.0 #1652
Conversation
…to v1.4.0 Upgrade dependency `github.com/go-sql-driver/mysql` from 2e00b5cd7039 (Dec 24, 2016), which is a couple commits after `v1.3.0`, to `v1.4.0` (Jun 3rd, 2018). The `mysql` module is used directly by this repositories `support/db` package. The `mysql` module is also used indirectly via the `github.com/jmoiron/sqlx` module which we have pinned to `v1.2.0`. We use the `sqlx` package in `services/ticker`, `services/horizon` and `support/db`. The `sqlx` package's tests have a dependency on `mysql` `v1.4.0` and even though it is only their tests, Modules carries that minimum requirement to our project, even though we are not dependent on the tests. ``` +-----------------------------+ | +------+ +-----------+ | +----------->+ sqlx +<-----+ sqlx.test | | | v1.2.0 | +------+ +-----------+ | | +-----------------------------+ +-----+------+ | | stellar/go | | +-----+------+ | | | | +-------+ | +----------->+ mysql +<----------+ +-------+ v1.4.0 ``` This isn't ideal for us because we don't particularly care about dependencies of our dependencies tests, but the perspective of the Go team has been that a dependencies test dependencies are a signal to compatability of the module in general. e.g. If the sqlx devs say that to test sqlx they need at least mysql v1.4.0, there's a good chance sqlx is only compatible with v1.4.0+. I considered two approaches to resolve this. 1. Upgrade `mysql` to `v1.4.0`, which is this change. 2. Downgrade `sqlx` two commits to drop it's `go.mod` file. The last two commits on the version we have are only the addition of a `go.mod` file. Without it we can pin the version of `mysql` that we have today. This isn't a good long term plan though, as sooner or later we're going to need to be bumped. I've gone with this change because it better embraces the best practices from the perspective of the core Go developers and it will probably cause us less problems long term. The diff in the mysql dependency is 69 commits, 38 changed files, 4,928 additions, 797 deletions, 12 new features, 12 bug fixes, and numerous other changes. It's not insignificant and is probably not possible for one of us to review and be able to meaningful identify if upgrading would be harmful. The best thing we can do is rely on our tests here.
…r/mysql_from_2e00b5cd7039_to_v1.4.0
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.
1.4 Changelog is here: https://github.com/go-sql-driver/mysql/blob/master/CHANGELOG.md#version-14-2018-06-03
It doesn't look like anything too major to me. Given that the dependency for sqlx is only in the tests, and the changes are mostly not breaking, I think we should try it out.
@ire-and-curses I want to make sure this isn't missed. The requirement to use v1.4.0 is only from the sqlx tests, but we do use the mysql dependency, so the dependency changing here will change the code we run in production. I've just re-requested your approval because I wanted to make sure this was communicated and I hadn't miscommunicated about this in the description. |
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.
Understood that we use it directly in support/db
- thanks for pointing that out. Let's proceed.
PR Checklist
PR Structure
otherwise).
services/friendbot
Thoroughness
This PR adds tests for the most critical parts of the new functionality or fixes.I've updated any docs (developer docs,.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
I've updated the relevant CHANGELOG (here for Horizon) ifneeded with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
Summary
Upgrade dependency
github.com/go-sql-driver/mysql
from2e00b5cd7039
(Dec24, 2016), which is a couple commits after
v1.3.0
, tov1.4.0
(Jun 3rd,2018).
Goal and scope
This is part of making the transition to Modules (#1634).
The
mysql
module is used directly by this repositoriessupport/db
package. The
mysql
module is also used indirectly via theg.yxqyang.asia/jmoiron/sqlx
module which we have pinned tov1.2.0
. Weuse the
sqlx
package inservices/ticker
,services/horizon
andsupport/db
. Thesqlx
package's tests have a dependency onmysql
v1.4.0
and even though it is only their tests, Modules carries thatminimum requirement to our project, even though we are not dependent on
the tests.
This isn't ideal for us because we don't particularly care about
dependencies of our dependencies tests, but the perspective of the Go
team has been that a dependencies test dependencies are a signal to
compatability of the module in general. e.g. If the sqlx devs say that
to test sqlx they need at least mysql v1.4.0, there's a good chance sqlx
is only compatible with v1.4.0+.
I considered two approaches to resolve this.
mysql
tov1.4.0
, which is this change.sqlx
two commits to drop it'sgo.mod
file. The last twocommits on the version we have are only the addition of a
go.mod
file.Without it we can pin the version of
mysql
that we have today. Thisisn't a good long term plan though, as sooner or later we're going to
need to be bumped.
I've gone with this change because it better embraces the best practices
from the perspective of the core Go developers and it will probably
cause us less problems long term. The diff in the mysql dependency is 69
commits, 38 changed files, 4,928 additions, 797 deletions, 12 new
features, 12 bug fixes, and numerous other changes. It's not
insignificant and is probably not possible for one of us to review and
be able to meaningful identify if upgrading would be harmful. The best
thing we can do is rely on our tests here.
Summary of changes
mysql
package tov1.4.0
.appengine
package that was added automatically as it's a dependency of themysql
package and was missing from the.lock
file. (This should be meaningless to us, unless we were to deploy to Google AppEngine.)Known limitations & issues
N/A
What shouldn't be reviewed
N/A