Skip to content

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

Merged
merged 2 commits into from
Aug 27, 2019
Merged

support/db: upgrade github.com/go-sql-driver/mysql from 2e00b5cd7039 to v1.4.0 #1652

merged 2 commits into from
Aug 27, 2019

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Aug 27, 2019

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    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) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    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 from 2e00b5cd7039 (Dec
24, 2016), which is a couple commits after v1.3.0, to v1.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 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.

Summary of changes

  • Update mysql package to v1.4.0.
  • Include addition of appengine package that was added automatically as it's a dependency of the mysql 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

…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.
@leighmcculloch leighmcculloch marked this pull request as ready for review August 27, 2019 19:01
Copy link
Contributor

@ire-and-curses ire-and-curses left a 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.

@leighmcculloch
Copy link
Member Author

@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.

Copy link
Contributor

@ire-and-curses ire-and-curses left a 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.

@leighmcculloch leighmcculloch merged commit e39f70c into stellar:master Aug 27, 2019
@leighmcculloch leighmcculloch deleted the issue_1634_upgrade_github.com/go-sql-driver/mysql_from_2e00b5cd7039_to_v1.4.0 branch August 27, 2019 20:03
@leighmcculloch leighmcculloch self-assigned this Sep 16, 2019
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