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 cache instead of vendor #959

Closed
wants to merge 1 commit into from
Closed

Use cache instead of vendor #959

wants to merge 1 commit into from

Conversation

antechrestos
Copy link
Contributor

@antechrestos antechrestos commented Jan 8, 2020

Description

This work is a proposal. In another pull request, I had more than 1K files modified just by changing a dependency version.

I took sight it might also be a good way to introduce an undesired/unhandled/malicious modification in vendor files as currently travis does not check consistency between vendor directory and go.mod file.

In this change I did the following things

  • remove the vendor directory
  • create a .travis/go-cachedirectory: to create this, I cleaned my own cache directory ( go clean --modcache), deleted the vendor directory, and did a go mod vendor. Then I created the .travis/go-cache directory from $GOPATH/pkg/moddirectory
  • updated .travis.yml, integration-test.sh to copy cache to $GOPATH/pkg/mod before running a go mod vendor
  • Updated DEVELOPMEND.md file to tell any neww developer to run a go mod vendor after cloning repository.
  • update the two script hack/boilerplate.sh and hack/gofmt.sh to ignore the .travis directory
  • added the vendordirectory to the .gitignore file

I cut my laptop from network and made a cp -r ./.travis/go-cache $GOPATH/pkg/mod && go mod vendor && make out/executorand it worked perfectly.

This way, it may be a good thing to let the core team be the protector of the .travis/go-cache directory (pull requests will download new dependencies in their jobs but cache will be updated by core team once it is merged).

What do you think of it @cvgw @tejal29 ?

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes [unit tests]not needed
  • Adds integration tests if needed. not needed

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Jan 8, 2020
This change aims to remove vendor directory in order to add more flexibility to changes in dependency versions but keep it for CI purpose
@tejal29
Copy link
Contributor

tejal29 commented Jan 10, 2020

@antechrestos Thank you for your PR.
I took a quick glance and have couple of questions.

  1. IIUC, we have just moved all the files in vendor to .travis/go-cache right?
  2. What happens when a developer adds a new dependency? They will have to add that to .travis/go-cache now or else travis ci will end up downloading this new dependency for every single run? So we will still have the same issue of large diffs when adding a new dependency?

Thanks
Tejal

@antechrestos
Copy link
Contributor Author

antechrestos commented Jan 10, 2020

@tejal29

Thank you for your feedback.

  1. More or less. In fact I just put in .travis/go-cache the cache generated by go when you generate vendordirectory with go mod vendor command. On your own computer you can see that in $GOPATH/pkg/modthere are both go sources and additional database used by go`
  2. On this point I would suggest that external contributors (by it I mean out of the collaborators) only change the go.mod file. Travis would end downloading new dependencies as long as collaborators does not update the local cache. This will lower the number of file changed in pull requests and prevent any malicious use of vendor libraries.

Ben

@cvgw
Copy link
Contributor

cvgw commented Jan 10, 2020

I'm not sure I follow the intent of this PR. Can we open an issue to discuss this further?

@tejal29
Copy link
Contributor

tejal29 commented Jan 17, 2020

@antechrestos At this time, i don't mind reviewing the larger diffs when dependencies are added Vs adding additional maintenance load of updating the travis cache.

In future we can re-evaluate this decision.
Sorry and thank you for bringing this up!

Thanks
Tejal

@tejal29 tejal29 closed this Jan 17, 2020
@antechrestos antechrestos deleted the feature/use_cache_instead_of_vendor_directory branch January 18, 2020 10:38
@antechrestos
Copy link
Contributor Author

@tejal29 Thank you for this feedback 😄 .
Good to know that there is a vigilance about dependencies.

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants