Skip to content

[6.x] update beats - #1054

Merged
simitt merged 6 commits into
elastic:6.xfrom
simitt:6.x-update-beats
Jul 3, 2018
Merged

[6.x] update beats#1054
simitt merged 6 commits into
elastic:6.xfrom
simitt:6.x-update-beats

Conversation

@simitt

@simitt simitt commented Jul 2, 2018 •
edited by zube Bot
Loading

Copy link
Copy Markdown
Contributor

This PR updates to the latest beats version 6.x, including following changes:

  • adapting make check to fail when the elastic license header is missing in any files, and adapts make fmt to add the missing license header in any file. Running those two commands added license headers to all files now.
  • govendor github.com/elastic/beats/libbeat/generator/fields for changed fields generation and handling.
  • generally updating beats libraries
  • updating github.com/elastic/go-struct to v0.0.4

simitt added 2 commits July 2, 2018 10:34
@simitt
Update beats framework to 39cd1ab
9177a1b
@simitt
Add license headers.
4bd0597 
Running `make fmt check` now adds license headers everywhere missing.
simitt
simitt commented Jul 2, 2018
View reviewed changes
Comment thread Makefile
.PHONY: fields
fields:
@cat _meta/fields.common.yml > _meta/fields.generated.yml
@cat processor/*/_meta/fields.yml >> _meta/fields.generated.yml

@simitt simitt Jul 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changes are aligned with changes made when updated to beats framework in master, see https://github.com/elastic/apm-server/pull/987/files#diff-b67911656ef5d18c4ae36cb6741b7965.

@simitt simitt added the review label Jul 2, 2018
graphaelli
graphaelli suggested changes Jul 2, 2018
View reviewed changes

@graphaelli graphaelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you backport 157a9e5 with this since it brings in generation of those files?

make update on this branch removes the license headers on processor/*/schema.go - I'm suprised that doesn't trigger a test failure.

Go update needs this treatment - https://github.com/elastic/apm-server/pull/894/files

@simitt

simitt commented Jul 2, 2018

Copy link
Copy Markdown
Contributor Author

make update should not remove the license headers, as I added a change https://github.com/elastic/apm-server/pull/1054/files#diff-2cd3c44030e450f619ef38d007f1ba75R59 to run the licensing tool after creating the schema files.

@simitt

simitt commented Jul 2, 2018

Copy link
Copy Markdown
Contributor Author

@graphaelli good findings! I've updated accordingly.

graphaelli
graphaelli reviewed Jul 2, 2018
View reviewed changes

@graphaelli graphaelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably go straight to 1.10.3 as beats has done on 6.x now.

Still seeing the headers removed on the generated schema files on make update

@simitt

simitt commented Jul 2, 2018

Copy link
Copy Markdown
Contributor Author

That is very weird. Do you maybe have the CHECK_HEADERS_DISABLED env variable set?
If not could you run make check once to ensure you have the licensing tool installed.

@simitt
simitt force-pushed the 6.x-update-beats branch from 5fa5fc5 to d77e81b Compare July 2, 2018 16:44
graphaelli
graphaelli approved these changes Jul 2, 2018
View reviewed changes

@graphaelli graphaelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That was it, go-licensor wasn't installed. Disappointing that it fails silently, but not a topic for this review. LGTM now (when green)

@jalvz

jalvz commented Jul 3, 2018

Copy link
Copy Markdown
Contributor

fyi im doing the 1.10.3 update on master as well also as part of updating beats

@simitt

simitt commented Jul 3, 2018

Copy link
Copy Markdown
Contributor Author

@graphaelli you are absolutely right to expect a failure when the go-licenser is not installed. I added a panic if the licenser cannot run. This can only panic when the schemas are built, which is at build and not at runtime.

@simitt
simitt force-pushed the 6.x-update-beats branch from 18c989b to 1163bad Compare July 3, 2018 06:39
@simitt

simitt commented Jul 3, 2018

Copy link
Copy Markdown
Contributor Author

update: I removed the go-license check from the inline-script again and just run the add-headers cmd within the make update. This way the logic does not get duplicated and it seems cleaner to trigger the go-licenser from within the Makefile. Also the go-licenser would add the headers to all files and not only the ones touched by the inline-script,

@simitt simitt self-assigned this Jul 3, 2018
jalvz
jalvz reviewed Jul 3, 2018
View reviewed changes
Comment thread CHANGELOG.asciidoc Outdated
==== Added

- Listen on default port 8200 if unspecified {pull}[886]886.
- Update Go to 1.10.3 {pull}894[894].

@jalvz jalvz Jul 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wrong PR number?

@simitt simitt Jul 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, because I first just backported this PR, but now changed from 1.10.1 to 1.10.3.
I adapt to reflect this PR number.

@simitt
simitt force-pushed the 6.x-update-beats branch from 1163bad to 5ded05d Compare July 3, 2018 07:25
@simitt
simitt merged commit 9e0ecc9 into elastic:6.x Jul 3, 2018
@simitt
simitt deleted the 6.x-update-beats branch July 10, 2018 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@jalvz jalvz jalvz left review comments
@graphaelli graphaelli graphaelli approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@simitt simitt

Labels

review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@simitt @jalvz @graphaelli @alvarolobato