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

Migrate to Go modules #3418

Merged
merged 15 commits into from
Mar 10, 2020
Merged

Migrate to Go modules #3418

merged 15 commits into from
Mar 10, 2020

Conversation

axw
Copy link
Member

@axw axw commented Mar 4, 2020

Motivation/summary

Use Go modules instead of vendoring. Modules are the future.

There are a bunch of changes here:

  • The vendor and _beats directories are removed. A couple of minor bash scripts, and the reviewdog.yml file, have been copied into our tree. They haven't been changed in a couple of years.
  • generate_notice.py has been forked to stop assuming vendoring. It now only considers modules that are used in the apm-server binary, and not those used for build/test purposes.
  • Updated beats import paths to use "beats/v7"
  • Updated Makefile and scripts to work with modules. A handful of scripts have been removed.
  • Updated to elastic/beats@bbf9d6697f4a, and adapted to some ES output-related breaking changes

Checklist

  • I have signed the Contributor License Agreement.
  • My code follows the style guidelines of this project (run make check-full for static code checks and linting)
  • I have rebased my changes on top of the latest master branch
  • I have made corresponding changes to the documentation
    - [ ] I have added tests that prove my fix is effective or that my feature works
    - [ ] New and existing unit tests pass locally with my changes
    - [ ] I have updated CHANGELOG.asciidoc (no user-facing changes)

How to test these changes

  • make
  • make docker-system-tests
  • make update-beats

Related issues

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

This is amazing work!

I tested the most important make cmds and they worked fine, except for running into the same issues as CI does for make docker-system-tests. Will do a final review once this is solved.

script/generate_notice.py Show resolved Hide resolved
@axw
Copy link
Member Author

axw commented Mar 4, 2020

I tested the most important make cmds and they worked fine, except for running into the same issues as CI does for make docker-system-tests. Will do a final review once this is solved.

@simitt thanks for taking a look. I'm working through CI issues one-by-one, I'll let you know when I think it's stable enough for reviewing again.

@axw
Copy link
Member Author

axw commented Mar 5, 2020

crossbuild is failing to locate the github.com/elastic/beats/v7 module directory:

[2020-03-04T14:27:13.282Z] panic: failed determine libbeat dir location: expected 1 line, got 0

[2020-03-04T14:27:13.282Z] 

[2020-03-04T14:27:13.282Z] goroutine 1 [running]:

[2020-03-04T14:27:13.282Z] github.com/elastic/beats/v7/dev-tools/mage.LibbeatDir(0xc0001c20b0, 0x1, 0x1, 0xc0000b6f80, 0x34)

[2020-03-04T14:27:13.282Z] 	/var/lib/jenkins/workspace/pm-server_apm-server-mbp_PR-3418/pkg/mod/github.com/elastic/beats/[email protected]/dev-tools/mage/common.go:742 +0x1d3

[2020-03-04T14:27:13.282Z] github.com/elastic/beats/v7/dev-tools/mage.init()

[2020-03-04T14:27:13.282Z] 	/var/lib/jenkins/workspace/pm-server_apm-server-mbp_PR-3418/pkg/mod/github.com/elastic/beats/[email protected]/dev-

This is because in the fresh container, the module cache hasn't yet been populated. We'll need to either share the module cache from the host (might be a bit dangerous?), or otherwise seed the container's module cache before calling mage.

@axw
Copy link
Member Author

axw commented Mar 6, 2020

elastic/beats#16837 is up to address the above issue. I went for mounting the module cache in, but made it read-only. Before starting the crossbuild containers, we run go mod download on the host to ensure the cache is up to date.

@axw
Copy link
Member Author

axw commented Mar 6, 2020

I lost the race with elastic/beats#16150. Will need to update to the latest API in this PR.

@axw axw changed the title Migrate to Go modules (WIP) Migrate to Go modules Mar 7, 2020
axw added 10 commits March 7, 2020 11:00
- Add go.mod
- Add tools.go for tracking build/test tools in go.mod
- Update Makefile to use go.mod, instead of vendor
- Update beats imports to use "beats/v7"
- Update cespare/xxhash to use "xxhash/v2"
- Copy reviewdog.yml from beats
- Copy build scripts from beats/dev-tools
- Modify "update-beats" target to rsync libbeat/testing/environments
  into apm-server/testing/environments. We could alternatively just
  go our own way, but this is fairly easy to maintain for now.

- Update tests/Dockerfile to remove GOPATH, and prime the container
  with the module dependencies by copying in "go.mod" and running
  "go mod download".
Handle libbeat/common/transport moves.

Closes elastic#3416
@axw axw requested a review from simitt March 7, 2020 03:03
@axw
Copy link
Member Author

axw commented Mar 7, 2020

@simitt sorry for the force push. I should have been clearer before, that I didn't intend for anyone to look at it while it was WIP. It was non-draft so that CI would run.

Should be good to review now, CI is passing. Every commit other than the first should be reviewable ;)

@axw axw requested a review from a team March 7, 2020 03:06
@axw
Copy link
Member Author

axw commented Mar 7, 2020

TODO:

  • Update README to remove govendor bits, replace with modules.

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Worked as expected locally.

Copy link
Contributor

@jalvz jalvz left a comment

Choose a reason for hiding this comment

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

Great to have this!

docs/fields.asciidoc Outdated Show resolved Hide resolved
testing/environments/5.x.yml Outdated Show resolved Hide resolved
@axw axw merged commit 45e8825 into elastic:master Mar 10, 2020
axw added a commit to axw/apm-server that referenced this pull request Mar 12, 2020
- Add go.mod
- Add tools.go for tracking build/test tools in go.mod
- Update Makefile to use go.mod, instead of vendor
- Update beats imports to use "beats/v7"
- Update cespare/xxhash to use "xxhash/v2"
- Copy reviewdog.yml from beats
- Copy build scripts from beats/dev-tools
- Remove vendor and _beats
- Fork generate_notice.py -- needed because we do not (can not) use vendoring
- Update beats framework to bbf9d6697f4a; adapt to several breaking changes in libbeat
- tests/system: use "go list" to find libbeat
- Modify "update-beats" target to rsync libbeat/testing/environments
  into apm-server/testing/environments. We could alternatively just
  go our own way, but this is fairly easy to maintain for now.
- Update tests/Dockerfile to remove GOPATH, and prime the container
  with the module dependencies by copying in "go.mod" and running
  "go mod download".
axw added a commit that referenced this pull request Mar 12, 2020
* Migrate to Go modules (#3418)

- Add go.mod
- Add tools.go for tracking build/test tools in go.mod
- Update Makefile to use go.mod, instead of vendor
- Update beats imports to use "beats/v7"
- Update cespare/xxhash to use "xxhash/v2"
- Copy reviewdog.yml from beats
- Copy build scripts from beats/dev-tools
- Remove vendor and _beats
- Fork generate_notice.py -- needed because we do not (can not) use vendoring
- Update beats framework to bbf9d6697f4a; adapt to several breaking changes in libbeat
- tests/system: use "go list" to find libbeat
- Modify "update-beats" target to rsync libbeat/testing/environments
  into apm-server/testing/environments. We could alternatively just
  go our own way, but this is fairly easy to maintain for now.
- Update tests/Dockerfile to remove GOPATH, and prime the container
  with the module dependencies by copying in "go.mod" and running
  "go mod download".

* Update to elastic/beats@9735c6d51540

The eslegclient changes have not been backported
to 7.x (yet?), so this commit reverts those changes.
@axw axw mentioned this pull request Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants