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

Moving from glide to govendor #3851

Merged
merged 1 commit into from
Mar 30, 2017
Merged

Moving from glide to govendor #3851

merged 1 commit into from
Mar 30, 2017

Conversation

vjsamuel
Copy link
Contributor

@vjsamuel vjsamuel commented Mar 30, 2017

Attempting to change go dependency management from glide to govendor.

This PR contains the following changes:

  • Set a fixed version for mysql and postgresql modules in metricbeat
  • Use govendor as dependency management instead of glide
  • Support for vendor directory in modules by exluding modules from auto discovery
  • Use auto discovery for global vendor directory
  • Add dependencies for testing

This PR didn't touch the dependencies in the docker module as they have some internal changes.

@vjsamuel
Copy link
Contributor Author

cc: @ruflin

@elasticmachine
Copy link
Collaborator

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine
Copy link
Collaborator

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

@vjsamuel vjsamuel force-pushed the govendor branch 5 times, most recently from 8304799 to 639204e Compare March 30, 2017 08:34
@ruflin ruflin added the review label Mar 30, 2017
@ruflin
Copy link
Member

ruflin commented Mar 30, 2017

I updated the PR text to add more details on the changes. Some things we should do / check:

  • Run make clean-vendor to remove some additional test files from sarama
  • It removes dependencies for github.com/andrewkroh/sys/plan9/env_plan9.go etc. I assume we still need these, potentially for packaging? @andrewkroh If yes, should be added manually.
  • Same as above for github.com/andrewkroh/sys/unix/* and github.com/andrewkroh/sys/windows/*
  • github.com/tsg/gopacket was removed
  • Some dependencies were added we didn't have in before:
    • github.com/blakesmith/ar
    • github.com/valiercorder/go-rpm
    • github.com/go-ole/go-ole
  • Some dependencies changed. We should check for the same commit id
    • github.com/dustin/go-humanize
    • github.com/davecgh/go-spew
    • github.com/eapache/go-resiliency
    • ...
  • Lots of files like .golden were removed.
  • Lots of files were removed as we only need certain packages and not the full library which govendor makes possible.

@ruflin
Copy link
Member

ruflin commented Mar 30, 2017

jenkins, test it

@ruflin
Copy link
Member

ruflin commented Mar 30, 2017

jenkins, package it

@ruflin
Copy link
Member

ruflin commented Mar 30, 2017

@ruflin
Copy link
Member

ruflin commented Mar 30, 2017

Just found out where the "new packages" are coming from. dev-tools also has its own vendor directory. We should add this also to the "exclude" list and clean it up seperately.

@ruflin
Copy link
Member

ruflin commented Mar 30, 2017

I found all of the above missing / added files and all looks good. From my perspective we need to have the following changes before merging this PR:

  • Add dev-tools/vendor to the ignore list and update the files again to not load these packages
  • Run make clean-vendor to remove some more files
  • Run make update

I'm ok with leaving in the the updated dependencies for some of the packages as we are in master and we need to update these anyways.

@elastic/beats WDYT?

@andrewkroh
Copy link
Member

  • It removes dependencies for github.com/andrewkroh/sys/plan9/env_plan9.go etc. I assume we still need these, potentially for packaging? @andrewkroh If yes, should be added manually.
  • Same as above for github.com/andrewkroh/sys/unix/* and github.com/andrewkroh/sys/windows/*

Of the github.com/andrewkroh/sys package, only github.com/andrewkroh/sys/windows/svc/eventlog is modified from the upstream. And it contains a small change to help with writing test cases for Winlogbeat. It's only used in test code.

@vjsamuel
Copy link
Contributor Author

@andrewkroh, if tests pass then does it mean that the change is good? I had a passing build until the previous commit. I've pushed a fix again. Build should pass now.

@ruflin ruflin merged commit eeb3083 into elastic:master Mar 30, 2017
@ruflin
Copy link
Member

ruflin commented Mar 30, 2017

@andrewkroh @vjsamuel I merged it. The great thing about govendor is that it only include ithub.com/andrewkroh/sys/windows/svc/eventlog directory from the complete fork which is great. Lets do some follow up PR's in case there are some more corrections needed.

@vjsamuel
Copy link
Contributor Author

👍

@vjsamuel vjsamuel deleted the govendor branch April 5, 2017 18:22
@tsg tsg mentioned this pull request Jul 24, 2017
28 tasks
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.

5 participants