-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Build docker and kubernetes features only on supported platforms #13509
Build docker and kubernetes features only on supported platforms #13509
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think import files shouldn't include the platforms as suffix. For the rest it looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @jsoriano has some good suggestions. After making those changes it should be good to go.
b24a9b4
to
43ac7ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It LGTM, just a couple of fixes in comments and a question about the test skipped in Windows.
What is the advantage of moving unit tests to system tests? |
@urso In a nutshell, I think the architectural changes require changes in testing: #13509 (comment) |
97f980f
to
88d3ace
Compare
I think it is somewhat unfortunate we need to introduce an extra registry for making processors available to JavaScript explicitely. Looks like an easy step to forget for future developers. If we want to stick with this, we should document in the processors registry that developers need to register with JavaScript as well + add a developer changelog entry. All in all +100 on removing docker/k8s from other platforms then linux/windows/darwin. |
@urso For the record, my changes do not add an extra registration step. Previously, if someone wanted to add a processor to JS processor, he/she had to add its constructor explicitly to a list of constructors in the processor. See: https://github.com/elastic/beats/pull/13509/files#diff-7dcd061ff7d5ba1d82f976b8ecdc4d73L47 I am adding a dev changelog entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you registered a mock processor for unit testing the script processor. I would like to see the "chain of chains" unit test added back to Go. I don't think its necessary to port most of test cases to Python because they don't add much value beyond testing the target processor (like add_locale) and that should already be covered by the target processors unit tests.
And maybe instead of keeping a separate registry for JS processors we could just have one processor registry and mechanically translate the names from underscore separated to upper-camel-case (with special cases for known acronyms). Like translate community_id
to CommunityID
or decode_json_fields
to DecodeJSONFields
. We do a translation like this in the beats asset
package and also in ECS (https://github.com/elastic/ecs/blob/f609a30b8f8cc18709e93a23d7990dfab5a58d5d/scripts/cmd/gocodegen/gocodegen.go#L297-L315).
@andrewkroh In this case, we still need to keep a list of valid processors for the JS one. It is true that this way we only have one registry and it possibly leads to a bit smaller memory footprint. However, we still need to maintain the list for JS processor. So from the developers' point of view, I don't see any additional value of this approach. |
I'd say let's postpone the JavaScript processor registration for later (separate PR), and keep the focus on docker/k8s here. Another idea for registering JavaScript processors would be the introduction of functional options:
In case we don't give a name, we could derive the name from the processor name and register like this:
All in all, all solutions have the same problem. The developer needs to remember to register a processor as JavaScript compatible. We can just keep it as it is right now. @andrewkroh is there a reason for not including all processors automatically? |
Opened issue to track the enhancements: #13652 So we can merge this PR. |
I think it should be find to include all of the processors with the exception of script processor itself. So we don't actually need to keep a list of "valid processors" for the JS processor. It's just a matter of getting the name formatted appropriately. |
main package to automatically register all of the standard supported Metricbeat | ||
modules. | ||
*/ | ||
// +build linux darwin windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kvch This build tag will not have any effect because it's preceeded by a block comment (rather than a line comment).
Constraints may appear in any kind of source file (not just Go), but they must appear near the top of the file, preceded only by blank lines and other line comments. https://golang.org/pkg/go/build/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a fix in #13654.
The build tags added in elastic#13509 didn't have an effect because only line comments can precede build tags. This also adds back cross-compilation for freebsd and netbsd that was disabled due to Docker compilation issues. Closes elastic#13400
As discussed in #13415 (comment), before updating
github.com/docker/docer
to a newer version, I am cleaning up building Docker related features. Thus, we can get rid of the forked upstream repo: https://github.com/exekias/moby.From now on
add_docker_metadata
,add_kubernetes_metadata
,kubernetes
anddocker
autodiscovery providers are only supported onlinux
,darwin
andwindows
.I added a separate plugin registry for the processor
javascript
, so there is no need to include other processors in it: https://github.com/elastic/beats/compare/master...kvch:fix-libbeat-compile-docker-on-supported-platforms?expand=1#diff-7dcd061ff7d5ba1d82f976b8ecdc4d73I also moved a few processor unit tests to system tests and added a mock processor to the JS processor instead.
Note for developers
If you would like to add your processor to the JS processor you need to register it during
init
when it is added to the "global" processor registry using itsRegisterPlugin
function.Example from
add_docker_metadata
: