-
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
New Filebeat subcommand: generate #9314
New Filebeat subcommand: generate #9314
Conversation
016f4c2
to
a220213
Compare
} | ||
|
||
genModuleCmd.Flags().String("modules-path", defaultHomePath, "Path to modules directory") | ||
genModuleCmd.Flags().String("es-beats", defaultHomePath, "Path to Elastic Beats") |
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.
Do we really need to expose both, because by default both of the flags will point to the same place?
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.
Hmm... Not yet. Btu when community Beats will support a similar module-handling, we will need this.
jenkins test this |
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, WFG
40f7599
to
e7901be
Compare
Rebased |
fa4afbc
to
ea07b1a
Compare
If we expose this to the user, I think we should also add some docs on how we expect the users to use this. How will the user handle the new fields? The PR has |
👍 on the docs, that's why the PR has the needs_docs label. What do you mean first question? Obviously, we don't "need" it, but we planned to release it in 6.x. Why do you think it is problem? |
A user creates a module with this command, how will he load / configure the additional fields that are used for it? If the user upgrades to a newer version, will the module stay intact? What if he wants the module on multiple machines? |
I am still not sure if I follow you. The behaviour of the generators has not changed. In this PR I only added new commands and subcommands to Filebeat to make existing functionality accessible to more people. After a user generates a module and a fileset, he/she can add the fields of the fileset to the appropriate Why wouldn't the module stay intact during an upgrade? If the name of the user's module is the same as a new module coming with the new release, it gets overwritten. But this has been the behaviour since introducing module generators in Filebeat. If he/she wants to have this module on multiple hosts, it is possible to copy them as before. Nothing changes here, also. |
Nice addition to Filebeat. An accompanying |
So far this functionality was only accessible to devs in the Beats repo and was not really intended for production but more for module development. With the above we make it accessible to all users and production use cases. One question I don't fully understand yet is what happens with the To be clear: I really like to make the above commands available to all our users but I want to make sure they have the experience they expect when they run the commands. |
Some parts of |
@kvch Even if To keep this moving forward few ideas:
To get this PR in quickly, we could start with 2 and see what the user feedback will be and work on 3. I personally like 3 because it will make modules "just work". |
@ruflin Indeed. I like what you have proposed in option 3. I will open a new PR with the changes which lets users read their local fields.yml file. That is a nice addition on its own. |
I only need to investigate why the test cannot find the test pipeline on Windows.... >.< |
jenkins test this |
d4baa47
to
649e95a
Compare
649e95a
to
a30035d
Compare
jenkins test this |
Failing tests are unrelated. |
Needs backport to 7.x branch... |
This PR exposes the existing Filebeat module generators as
generate
subcommand.The subcommands
module
,fileset
andfields
use the same code as the scripts.I added E2E tests to validate the functionality.
Blocked by #9147 as it contains the refactorings of that PR.