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

services/horizon: make fork of throttled primary dependency #1636

Closed
wants to merge 2 commits into from
Closed

services/horizon: make fork of throttled primary dependency #1636

wants to merge 2 commits into from

Conversation

leighmcculloch
Copy link
Member

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

Make @bartekn's fork of the throttled dependency the primary dependency for that dependency instead of being just a custom source.

Goal and scope

This is part of the initiative to move to Modules (#1634) and is a small change to simplify our Gopkg.toml/lock files to make that transition simpler.

Using the fork seems to be confusing the go tooling in go1.12 and go1.13rc1, with both behaving a little differently. Since this fork is the only fork of a dependency we use and it is simple package, it is straight forward for us to switch to using the fork fully in name rather than to try and make it work consistently with both versions.

For context, in modules a replace directive is how we tell the go toolchain that we want to use a fork of a module. However, a replace statement only applies when go commands are executed inside this repo as the main module and so importers of our repo won't be using the fork. This doesn't really matter for throttled because we only use it in internal and main modules of horizon, but it's important to be aware of.

  1. In one test I ran on go1.12.9 where this repo was being imported, all the dependencies for the project were imported and the tool got confused and stuck on importing a version of throttled because the commit only existed in the fork. It was trying to get it from the source repo because it was ignoring the replace directive. Go1.13 doesn't attempt to pull all dependencies like go1.12 does so this won't be a problem in the future, but go1.12 will be around for ~7 months from today.

  2. In another test I ran inside just this repo while pulling in all the dependencies locally the go toolchain saw the reference back in the fork in the example_test.go file and then attempted to pull in the source. This results in the project pulling in additional dependencies. This might be a bug, but I haven't been able to clearly reproduce it to open a ticket on the Go project.

In general we've been using this fork long term and the change has never been merged back upstream. Given the divergence I think in principle it's fine if switch to using the full name of @bartekn's fork and treating it as the primary dependency, and that is a smaller effort vs attempting to make this one case we use a fork work with a replace directive. Removing the need for a replace directive keeps our future go.mod file simple, which as newbies to Modules will hopefully mean we will have less problems and complexity to deal with.

Summary of changes

  • Replace github.com/throttled/throttled as a dependency with github.com/bartekn/throttled so that it is the name of the dependency and not just the source used to retrieve the dependency.
  • Replace github.com/throttled/throttled in import paths with github.com/bartekn/throttled.
  • Change the revision of github.com/bartekn/throttled to include an additional commit that @bartekn committed yesterday that removed the only self-referencing import line from an example test that we don't need. This was so that the original throttled package doesn't become a dependency.

Known limitations & issues

  • This pattern of entirely replacing a package with a fork isn't a pattern we can use in every situation. If we were using a fork of a more complex package with sub-packages those sub-packages would still reference it. This is a pattern we have the luxury of using with this simple package, and if the package was more complex we'd have to find a more complex solution or possibly work with the developers of the go toolchain to identify the precise way forward.

What shouldn't be reviewed

[TODO]

Make fork of throttled the primary dependency for that dependency
instead of being just a custom source.

When switching to Modules the custom source will become a replace
statement that only applies when go commands are executed inside
this repo as the main module. When go commands are executed in other
projects that import this repo this dependency will cause failures
because the commit sha doesn't exist in the original repository. Since
the fork has diverged long-term and there aren't plans to switch back,
we should make it the primary dependency.
@leighmcculloch leighmcculloch requested a review from bartekn August 22, 2019 21:07
@leighmcculloch
Copy link
Member Author

leighmcculloch commented Aug 22, 2019

🤦‍♂ I missed there were other files in @bartekn's fork that still reference throttled/throttled, and due to a typo in a test I ran, I wasn't testing it properly. CI pointed this out. Will close and reassess.

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.

1 participant