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 some "dep check" issues #1826

Merged
merged 4 commits into from Mar 7, 2019

Conversation

4 participants
@reggieriser
Copy link
Contributor

reggieriser commented Mar 6, 2019

Description

I hooked up the dep integration in GoLand this morning and it started complaining to me about a few dep things. Ran a dep check and it noted we were out of sync on gobuffalo/uuid and mitchellh/mapstructure. The former actually is deprecated, so I've fixed the references to it. The latter was added recently, but the Gopkg.lock wasn't updated. Those have been fixed by running dep ensure (after make clean and make server_generate).

There was another issue it noted:

# vendor is out of sync:
github.com/stretchr/testify: hash of vendored tree not equal to digest in Gopkg.lock

That hash was fixed by the dep ensure, but I noticed that /stretchr/testify shows version 1.3.0 in the Gopkg.lock file but version 1.2.2 in the Gopkg.toml file. Maybe that was just a timing issue between when the updater changed the version in Gopkg.lock and the explicit version was added to Gopkg.toml.

In addition, I get this warning when running dep ensure:

Warning: the following project(s) have [[constraint]] stanzas in Gopkg.toml:

✗ github.com/stretchr/objx

However, these projects are not direct dependencies of the current project:
they are not imported in any .go files, nor are they in the 'required' list in
Gopkg.toml. Dep only applies [[constraint]] rules to direct dependencies, so
these rules will have no effect.

Either import/require packages from these projects so that they become direct
dependencies, or convert each [[constraint]] to an [[override]] to enforce rules
on these projects, if they happen to be transitive dependencies.

I don't see any direct usages of objx in the codebase. @chrisrcoles, do you have any insight into this stretchr stuff? It looks like you added the Gopkg.toml constraints.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #1826 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1826   +/-   ##
=======================================
  Coverage   49.41%   49.41%           
=======================================
  Files         427      427           
  Lines       18352    18352           
  Branches     1629     1629           
=======================================
  Hits         9068     9068           
  Misses       8486     8486           
  Partials      798      798
@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

Let's also update the Gopkg.toml file. I have also asked @chrisrcoles before what to do about this objx dependency. We really need to figure out what's going on with it and fix the error/warning.

@mkrump

This comment has been minimized.

Copy link
Contributor

mkrump commented Mar 6, 2019

That hash was fixed by the dep ensure, but I noticed that /stretchr/testify shows version 1.3.0 in the Gopkg.lock file but version 1.2.2 in the Gopkg.toml file. Maybe that was just a timing issue between when the updater changed the version in Gopkg.lock and the explicit version was added to Gopkg.toml.

The Gopkg.toml version constraint has somewhat misleading syntax imo. https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md#version

Note: When you specify a version without an operator, dep automatically uses the ^ operator by default.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

chrisgilmerproj commented Mar 6, 2019

Can we specify operators then?

@reggieriser

This comment has been minimized.

Copy link
Contributor Author

reggieriser commented Mar 6, 2019

OK, that makes sense as to why version 1.3.0 was OK in the lock file -- 1.2.2 is going to match anything prior to 2.0. I'm curious what our intention was, though, with the version number constraints for the two stretchr libraries. The operator we use would depend on that.

Also, to prevent future sync issues, it looks like we could put a dep check pre-commit hook in place pretty easily if we can get this sorted out.

@reggieriser reggieriser requested a review from Igarfinkle Mar 7, 2019

@reggieriser

This comment has been minimized.

Copy link
Contributor Author

reggieriser commented Mar 7, 2019

Merged with master and fixed a new sync issue with github.com/honeycombio/beeline-go/trace.

@chrisrcoles

This comment has been minimized.

Copy link
Contributor

chrisrcoles commented Mar 7, 2019

Hey @reggieriser - there's no explicit need to lock the versions down for github.com/stretchr/objx and github.com/stretchr/testify. We can remove these references in the Gopkg.toml file. These two dependencies were added as a result of adding the mock generation and mock testing assertion work for the service object work for the CreateForm service object work. Although, I'm not exactly sure how these were added to the Gopkg.toml, removing those references should clean everything up.

@chrisrcoles
Copy link
Contributor

chrisrcoles left a comment

Let's remove references to github.com/stretchr/objx and github.com/stretchr/testify from the Gopkg.toml file.

@reggieriser

This comment has been minimized.

Copy link
Contributor Author

reggieriser commented Mar 7, 2019

Thanks, @chrisrcoles! I removed the stretchr libraries from Gopkg.toml and all still seems to be well. Could you and @chrisgilmerproj please remove your blocks and approve now?

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

Thanks for getting all this cleaned up. Looking forward to having a pre-commit hook for this too!

@reggieriser reggieriser merged commit 1496f09 into master Mar 7, 2019

19 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 38ce009...46e5a1e
Details
codecov/project 49.41% remains the same compared to 38ce009
Details

@reggieriser reggieriser deleted the rr-fix-dep-issues branch Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.