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

Fix go.sum #140

Closed
wants to merge 1 commit into from
Closed

Fix go.sum #140

wants to merge 1 commit into from

Conversation

fsouza
Copy link
Contributor

@fsouza fsouza commented Dec 18, 2018

See golang/go#29278.
Fix #138.

@dhui
Copy link
Member

dhui commented Dec 18, 2018

go version go1.11.4 linux/amd64
Builds are failing due to the invalid checksum: https://travis-ci.org/golang-migrate/migrate/jobs/469750108

@dhui
Copy link
Member

dhui commented Dec 18, 2018

Not sure if the checksums are failing due to the TravisCI build cache. However, I've cleared my module cache locally and rebuilt migrate and still have the same docker/docker checksum, so the TravisCI build cache is unlikely to be the issue.

@fsouza
Copy link
Contributor Author

fsouza commented Dec 18, 2018

Oh noo, I can't reproduce with a clean setup :(((

$ docker run -v $PWD:/code -w /code -it golang:1.11 make test COVERAGE_DIR=/tmp/coverage

Output here: https://gist.github.com/fsouza/f8fba9869dbd0ee0add12ce1b2897d06

I'll investigate this later today or some time tomorrow. Thanks for the feedback and sorry about the noise!

@fsouza
Copy link
Contributor Author

fsouza commented Dec 19, 2018

@dhui it does look like it may be a caching issue with Travis. I enabled Travis on my fork and go was able to download github.com/docker/docker: https://travis-ci.org/fsouza/migrate/jobs/469813717

An easy "fix" would be to update docker/docker to a newer version and see if the test suite stills passes, this would avoid caching issues that other people may face. We could pin it to the commit right after the current one or we could try to get the latest master and see if everything still works. What do you think?

@coveralls
Copy link

coveralls commented Dec 19, 2018

Pull Request Test Coverage Report for Build 241

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.728%

Totals Coverage Status
Change from base Build 240: 0.0%
Covered Lines: 676
Relevant Lines: 1171

💛 - Coveralls

@dhui
Copy link
Member

dhui commented Dec 19, 2018

Updating docker to a newer released version to get around this issue sounds good. We're due for an update anyways...
It's unclear to me if we need to use docker/docker or docker/engine to pin to a release.

This is an upgrade to the commit that bumped the Docker API to 1.39.
Upgrading the lib is actually a workaround for the go.sum + caching
issue.

Fixes #138.

For more details on the issue with checksums, see golang/go#29278.
@fsouza
Copy link
Contributor Author

fsouza commented Dec 19, 2018

@dhui yeah unfortunately after moving from docker/docker to moby/moby, the Docker team stopped tagging that repo, so we have to keep picking a commit :(

I pinned it to moby/moby@b6242da. This should be good for review now.

@dhui
Copy link
Member

dhui commented Dec 20, 2018

I'd rather avoid picking a commit and use tagged releases. That way, it's clear what version of a dependency we're using.

@fsouza
Copy link
Contributor Author

fsouza commented Dec 20, 2018

There are no tagged releases on docker/docker.

@dhui
Copy link
Member

dhui commented Dec 20, 2018

Try docker/engine

@fsouza
Copy link
Contributor Author

fsouza commented Dec 20, 2018

It's not possible to import github.com/docker/engine in the code, so this would require a replace in go.mod (I think):

replace github.com/docker/docker => github.com/docker/engine

I can do it, but it's very ugly :) Let me know what you think.

@dhui
Copy link
Member

dhui commented Dec 20, 2018

What are the issues you encountered when importing github.com/docker/engine?
I believe that migrate cannot use the replace directive since the directive is only used by the main module. e.g. migrate is used both as a CLI and a library

@dhui
Copy link
Member

dhui commented Dec 20, 2018

@dhui https://github.com/docker/engine/blob/170ed8d7e732adabe9b452a9a52c7a104ef2b8ec/api/types/container/config.go#L1

Oi... Thanks for investigating! In that case, let's stick with the docker/docker import and use commits which correspond to a release tag in docker/engine.
We can start with commit f5749085e9cb0565afe342e73a67631f97547054 which corresponds to tag v18.09.0, but if that has issues, we can use older versions (unless you wanna try to fix any issues that arise)

@fsouza
Copy link
Contributor Author

fsouza commented Dec 20, 2018

Yeah, unfortunately that commit is not in the tree referenced by master on docker/docker :( It's available only on docker/engine's 18.09 branch tree :(

docker/docker@master fails because that's requesting API version 1.40 which isn't released yet, so I pinned to the version that's using API 1.39.

@dhui
Copy link
Member

dhui commented Dec 21, 2018

Weird, the docs say that v18.09 supports API v1.30.

Next version to try would be v18.06.1-ce which is commit: 320063a2ad06a1d8ada61c94c29dbe44e2d87473

@fsouza
Copy link
Contributor Author

fsouza commented Dec 21, 2018

Sorry, this strategy isn't gonna work. The tags on docker/engine point to commits that do not exist on docker/docker's tree (though GitHub can render them under docker/docker because docker/engine is a fork).

As an alternative, I tried github.com/docker/docker@master but that didn't work because it's too far ahead, so I dug the commit history and picked the commit for 1.39, and that's what this PR is using.

@dhui
Copy link
Member

dhui commented Dec 21, 2018

Sorry, this strategy isn't gonna work. The tags on docker/engine point to commits that do not exist on docker/docker's tree (though GitHub can render them under docker/docker because docker/engine is a fork).

Oh, I thought docker/engine was a normal fork of docker/docker. I didn't realize that the commits were re-written...

As mentioned earlier, I'd rather not pin to arbitrary commit. I'm not familiar enough with the docker package to know which commits are considered stable (e.g. not an alpha/beta/RC release) and don't have a backwards incompatible changes. It'd also be unclear which commit to use for minor bug fixes.

Given the state of the docker/docker package, I think it's better postpone upgrading docker/docker.

I've also figured out the issue I was having with the checksum being being regenerated correctly locally. Earlier, I was using Go 1.11.3 instead of Go 1.11.4... I've now updated to Go 1.11.4 and regenerated go.sum in this commit: 2417646

Thanks again @fsouza for all of the work you've put in to resolving this issue!

@fsouza
Copy link
Contributor Author

fsouza commented Dec 21, 2018

Sounds good. Thanks for the feedback and sorry about the back and forth! :)

@fsouza fsouza closed this Dec 21, 2018
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.

3 participants