Skip to content

Adds all lib dependencies in vendor/ #181

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
Dec 14, 2017
Merged

Adds all lib dependencies in vendor/ #181

merged 2 commits into from
Dec 14, 2017

Conversation

xlr-8
Copy link
Contributor

@xlr-8 xlr-8 commented Dec 6, 2017

Issue Type

  • Enhancement

Summary

In order to reproduce build safely and all rely on the same library versions, it is best to keep the go dependencies versioned within the project. The Makefile has been adapted to avoid running fm/vet & lint against the vendor files.

The code was tested from scratch using a golang container.

  >  docker run -it golang:1.9 bash

  root@64d26bedf772:/go# git clone https://github.com/cristim/autospotting src/autospotting
  Cloning into 'src/autospotting'...
  remote: Counting objects: 4635, done.
  remote: Compressing objects: 100% (1688/1688), done.
  remote: Total 4635 (delta 705), reused 3282 (delta 701), pack-reused 1338
  Receiving objects: 100% (4635/4635), 9.83 MiB | 5.23 MiB/s, done.
  Resolving deltas: 100% (1514/1514), done.

  root@64d26bedf772:/go# cd src/autospotting/

  root@64d26bedf772:/go/src/autospotting# git fetch -p && git checkout hr-154
  Branch hr-154 set up to track remote branch hr-154 from origin.
  Switched to a new branch 'hr-154'

  root@64d26bedf772:/go/src/autospotting# go get -u github.com/golang/lint/golint

  root@64d26bedf772:/go/src/autospotting# make full-test
  ok      all files passed go fmt 
  ok      all files passed go vet 
  ok      autospotting/core       0.011s  coverage: 71.2% of statements

  root@64d26bedf772:/go/src/autospotting# exit

Closes: https://github.com/cristim/autospotting/issues/154

Code contribution checklist

  1. The contribution fixes a single existing github issue, and it is linked
    to it.
  2. The code is as simple as possible, readable and follows the idiomatic Go
    guidelines.
  3. No issues are reported when running make full-test.
  4. A code reviewer reproduced the problem and can confirm the code
    contribution actually resolves it.

@xlr-8
Copy link
Contributor Author

xlr-8 commented Dec 6, 2017

Also the check_deps rule is buggy, it didn't see that golint was missing in the container, I'll open an issue for that. created https://github.com/cristim/autospotting/issues/188

UPDATE: remove comments based on golint as PR was updated.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 72.294% when pulling 678e6d9 on hr-154 into 2c9e019 on master.

@xlr-8 xlr-8 self-assigned this Dec 6, 2017
@LeanerCloud LeanerCloud deleted a comment from coveralls Dec 6, 2017
Copy link
Member

@cristim cristim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is awesome, huge thanks!

It is one of the things that are critical to the reliability of the project, making sure our builds are consistent.

I don't have time to try it out but I trust you on this.

@xlr-8
Copy link
Contributor Author

xlr-8 commented Dec 6, 2017

I'll wait a bit before merging it, there isn't much to review - I wanted to ensure everything ran smoothly in a fresh environment but I didn't get the chance to test it tonight. I have reviewed other issues instead, I'll try it out later before merging

@xlr-8
Copy link
Contributor Author

xlr-8 commented Dec 7, 2017

I was wondering if the eawsy project was packaging the vendor as part of the archive. I inspected the archive but didn't learn much; according to this issue it should - however I'd like to be certain. Then I'll give it go on a small environment.

@xlr-8
Copy link
Contributor Author

xlr-8 commented Dec 13, 2017

Well considering that it builds the binary based on the sources, I believe we can merge it.

Hugo Rosnet added 2 commits December 14, 2017 00:23
In order to reproduce build safely and all rely on the same library
versions, it is best to keep the go dependencies versioned within the
project.
In order to avoid testing the govendor code (fmt,vet & lint), the rules
had to be revised.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.455% when pulling 9774069 on hr-154 into 053135e on master.

@cristim cristim merged commit aaacc9a into master Dec 14, 2017
@xlr-8 xlr-8 deleted the hr-154 branch December 15, 2017 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement vendoring of dependencies
3 participants