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

Use vendoring tool dep instad of committing the vendor folder #172

Merged
merged 3 commits into from
Aug 23, 2018

Conversation

mavimo
Copy link
Collaborator

@mavimo mavimo commented Aug 16, 2018

The repository already contains Gopkg.toml and Gopkg.lock but the vendoring folder is committed in codebase.

  • update Gopkg.toml and Gopkg.lock to use the last version of packages (see
    2b7d2a2)
  • introduce dep as vendoring tool in CI and build process (see 1fa718f)
  • remove /vendor folder from repository (see f174d07)

I still using dep since is the tool used in this repo but maybe we should consider to move to module when go1.11 will be released

@JohnnyQQQQ JohnnyQQQQ requested a review from xetys August 20, 2018 13:42
Copy link
Owner

@xetys xetys left a comment

Choose a reason for hiding this comment

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

As I'm inexperienced with dep, this looks good to me

@xetys xetys merged commit 0d56ff6 into xetys:master Aug 23, 2018
@mavimo mavimo deleted the use-vendoring-tool branch August 23, 2018 17:59
@slintes
Copy link

slintes commented Aug 28, 2018

too late, but: afaik most projects commit vendor, because the pros are stronger than the cons: https://github.com/golang/dep/blob/master/docs/FAQ.md#should-i-commit-my-vendor-directory

just my 2 cents :)

@JohnnyQQQQ
Copy link
Collaborator

@slintes I agree with you

@xetys
Copy link
Owner

xetys commented Aug 28, 2018

I was on the same site when deciding to commit it in. I merged that PR as I am not that experienced with the go ecosystem at all and it made sense at that moment.

So I would do a little vote here, at least @mavimo @JohnnyQQQQ @slintes, maybe @eliasp

React on this comment with

  • 👍 if you feel including the vendor dir is better, or
  • 👎 if you feel excluding the vendor dir as of this PR is better

@xetys
Copy link
Owner

xetys commented Aug 28, 2018

addition: if we choose to commit it back, I still would like to have that in an extra PR, as the updates to the Gopkg files were needed anyway

@JohnnyQQQQ
Copy link
Collaborator

I spun up a PR for using the go.mod approach introduced with golang 1.11 #181

@mavimo
Copy link
Collaborator Author

mavimo commented Aug 28, 2018

I suggest to use go mod for golang 1.11 and dep for go<=1.10

Dependency vendoring imho should not be done since we are going to introduce dependency in the codebase that should be omitted (we have technology that allow us to do that 😃)

The only issue that this generate is the requirement to download dependency when we pull the repo, that IMHO is acceptable for a contributor.

@JohnnyQQQQ
Copy link
Collaborator

Dependency vendoring imho should not be done since we are going to introduce dependency in the codebase that should be omitted

Can you clarify what you mean?

@mavimo
Copy link
Collaborator Author

mavimo commented Aug 28, 2018

@JohnnyQQQQ sorry :P
I mean we should not commit vendor dependency into the project repository, but should managed as dependency like other language/technology (eg: mvn for Java, composer for PHP, npm for Javascript, ...).

Duplicating the vendor code in our project is an anti-pattern that IMHO should not be done in any project.

@slintes
Copy link

slintes commented Aug 28, 2018

2 of the 3 the con arguments of getcomposer (history duplication, git submodule problems) are not relevant because go dep doesn't add the .git directory of dependencies into the vendor directory.
And the best pro argument is coming from the javascript/npm world: when someone deletes a project you depend on, you still have the code and can compile, until you find a replacement (https://www.theregister.co.uk/2016/03/23/npm_left_pad_chaos/).

(but at the end I don't care that much, this topic only catched my eyes while cleaning up my github notification mails)

@JohnnyQQQQ
Copy link
Collaborator

Never thought about someone deleting a whole project I depend on 😨

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.

4 participants