Move existing Filebeat generator logic to package generator
#7506
Conversation
kvch
added
review
refactoring
Filebeat
|
|
||
| replace := map[string]string{"module": module} | ||
| templatesPath := path.Join(beatsPath, "scripts", "module") | ||
| filesToCopy := []string{ |
There was a problem hiding this comment.
Reminds me a lot of https://github.com/elastic/beats/blob/master/dev-tools/mage/copy.go Perhaps we can share code somehow.
There was a problem hiding this comment.
it calls generator.CopyTemplates which not only copies the files, but substitutes the template strings in copied files. I think apart from copying, there is not so much to share.
I could move Copy from the package mage to libbeat/utils or libbeat/common/utils and use it from there. It would be a bit bigger undertaking, as right now files are copied one-by-one and after each file is copied, the substitution is done. If I use this, I need to change the algorithm to substitute template strings after the file is copied. It would also mean that each file would need to be opened, read and written twice, once to copy it and once to substitute strings.
However, if I do so, I would rather refactor all generators and other possible places where files are copied. So I would rather address this in a follow up PR. WDYT?
| return fmt.Errorf("module already exists: %s", module) | ||
| } | ||
|
|
||
| err := generator.CreateDirectories(modulePath, []string{path.Join("_meta", "kibana", "6")}) |
There was a problem hiding this comment.
I'm asking myself what version should it be here. And if we have the module generator as part of the beat, should we directly create the dashboard directory. Some users could also use 5.x or in the future 7.x with this beat version.
There was a problem hiding this comment.
Should we make it configurable? Maybe passing a param using a flag e.g. --kibana-version? It could be 6 by default until we release version 7.
There was a problem hiding this comment.
Instead of having the module generator creating the correct directory, it perhaps should be the kibana exporting command that takes care of the directory creation. So we don't create the directories here but they are only add when someone actually exports dashboards. It would be mean we have to add some knowledge about modules and Kibana directories to the exported but I think that would make sense.
There was a problem hiding this comment.
I think it's a good idea.
I am wondering if there are use cases when a users would need directories without calling export. But I don't see any.
There was a problem hiding this comment.
When would you create the directory? The exporter AFAIK prints the exported dashboard to stdout. Do you want it to change it to saving it to a file?
There was a problem hiding this comment.
yes, I think we should change the exported. Either to have default to file with the correct file structure or having a config option.
There was a problem hiding this comment.
Note: I would not change the exporter in this PR.
kvch
force-pushed
the
refactor/filebeat/move-fields-yml-generator
branch
from
e6ebaf7
to
bb163fe
Compare
|
How does this PR conflict with #7533 ? |
|
I am moving the code I edit in the other PR to a different package in this one. |
|
I'm ok with moving the refactoring to an other PR and merge this one. The reason I think it does not fit well into #7506 is because there a large part of the code is also moved around which will make it very hard to follow the diff. So my preferred option would be to have a follow up PR of this that takes care of the refactoring and then #7506 is rebase on top of it. #7506 is the reason for me the refactoring becomes more important as the code is moved from dev only usage to production usage. |
|
Works for me. |
|
Ups, my comment above was intended to be on #7533. That is why I referenced this PR. Let's sync up offline on the order and cleanup of the PR's. |
|
Closing in favour of a new PR. |
I am splitting up #6912 to make rebasing it a bit easier.
This PR moves the current generator logic, including module, fileset and fields.yml generators, to a different package.
Blocks #6912