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

Add vendoring with govendor #2461

Merged
merged 1 commit into from Sep 18, 2016

Conversation

Projects
None yet
3 participants
@moorereason
Contributor

moorereason commented Sep 16, 2016

Doesn't check in vendored packages. Rewrites Makefile to use govendor helpers.

@moorereason moorereason changed the title from WIP: Add vendoring to Add vendoring with govendor Sep 16, 2016

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 17, 2016

Member

The vendor.json file; is that a standard format?

Member

bep commented Sep 17, 2016

The vendor.json file; is that a standard format?

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Sep 17, 2016

Contributor

There is no standard format. That's govendor's format. The only standard is that the Go tools look for vendored packages in the vendor/ directory.

Contributor

moorereason commented Sep 17, 2016

There is no standard format. That's govendor's format. The only standard is that the Go tools look for vendored packages in the vendor/ directory.

@moorereason moorereason added this to the v0.17 milestone Sep 18, 2016

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Sep 18, 2016

Contributor

Tagged for 0.17. FIxes #2119.

Contributor

moorereason commented Sep 18, 2016

Tagged for 0.17. FIxes #2119.

@bep bep merged commit 6e692f2 into gohugoio:master Sep 18, 2016

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/wercker Wercker build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 18, 2016

Member

I merged this as it does solve the problem ... but we should do an evaluation of the vendoring options for 0.18.

Member

bep commented Sep 18, 2016

I merged this as it does solve the problem ... but we should do an evaluation of the vendoring options for 0.18.

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 18, 2016

Contributor

When I read the release notes for 0.17, I just expected the code under vendor/ to be checked in.

Should make govendor be mentioned in the README?

Contributor

nathany commented Oct 18, 2016

When I read the release notes for 0.17, I just expected the code under vendor/ to be checked in.

Should make govendor be mentioned in the README?

@@ -34,39 +32,45 @@ docker:
docker cp hugo-build:/go/bin/hugo .
docker rm hugo-build
govendor:
go get -u github.com/kardianos/govendor
go install github.com/kardianos/govendor

This comment has been minimized.

@nathany

nathany Oct 18, 2016

Contributor

FYI

"Get downloads the packages named by the import paths, along with their
dependencies. It then installs the named packages, like 'go install'." - go get -h

@nathany

nathany Oct 18, 2016

Contributor

FYI

"Get downloads the packages named by the import paths, along with their
dependencies. It then installs the named packages, like 'go install'." - go get -h

This comment has been minimized.

@nathany

nathany Oct 18, 2016

Contributor

Which is to say the go install line is superfluous.

It still appears here atm: https://github.com/spf13/hugo/blob/master/Makefile#L37

@nathany

nathany Oct 18, 2016

Contributor

Which is to say the go install line is superfluous.

It still appears here atm: https://github.com/spf13/hugo/blob/master/Makefile#L37

This comment has been minimized.

@moorereason

moorereason Oct 18, 2016

Contributor

You're right. I was looking at this on my phone and misunderstood where your comment was in the Makefile. The go install does indeed need to be removed.

@moorereason

moorereason Oct 18, 2016

Contributor

You're right. I was looking at this on my phone and misunderstood where your comment was in the Makefile. The go install does indeed need to be removed.

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Oct 18, 2016

Contributor

The Makefile has already been fixed in master.

And yes, we may need to update the README.

Contributor

moorereason commented Oct 18, 2016

The Makefile has already been fixed in master.

And yes, we may need to update the README.

@moorereason moorereason deleted the moorereason:govendor branch Mar 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment