Add support for ES index-template configs #552
Conversation
|
@skh @neptunian How would we implement this best on the Kibana side? |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
|
| Title string `config:"title" json:"title" validate:"required"` | ||
| Release string `config:"release" json:"release"` | ||
| Type string `config:"type" json:"type" validate:"required"` | ||
| IngestPipeline string `config:"ingest_pipeline,omitempty" config:"ingest_pipeline" json:"ingest_pipeline,omitempty" yaml:"ingest_pipeline,omitempty"` |
There was a problem hiding this comment.
Now that we have an Elasticsearch field, should IngestPipeline be moved under it?
There was a problem hiding this comment.
Good point. I would say yes!
There was a problem hiding this comment.
Will such a change pose a problem in terms of BWC for existing manifests?
There was a problem hiding this comment.
Yes and no. After 7.9 we must stop doing these changes but until then we can still update old ones. The way I plan such changes are:
- Update registry and support both format. Read in and output.
- Update Kibana and packages / integrations scripts
- Remove legacy code
@ycombinator Any help getting this changes in is appreciated ;-)
There was a problem hiding this comment.
Maybe I'm missing something about the plan but shouldn't there be an IngestPipeline field in the Elasticsearch struct below?
There was a problem hiding this comment.
Oh I see, the plan is to introduce it in #564.
util/dataset.go
Outdated
| @@ -84,6 +85,11 @@ type Os struct { | |||
| Windows interface{} `config:"windows" json:"windows,omitempty" yaml:"windows,omitempty"` | |||
| } | |||
|
|
|||
| type Elasticsearch struct { | |||
| IndexTemplateSettings map[string]interface{} `config:"index-template.settings" json:"index-template.settings,omitempty" yaml:"index-template.settings,omitempty"` | |||
There was a problem hiding this comment.
Nit: consistency. For ingest pipelines we use ingest_pipeline (underscore-delimited) but for index templates we use index-template (hyphen-delimited).
There was a problem hiding this comment.
Glad you bring this up. I remember we had in the past a discussion that we should standardise. You have any preference?
There was a problem hiding this comment.
tl;dr: I lean towards underscore.
When it comes to defining these fields in JSON or YAML I don't think it matters much, especially since we would never start a field name with a delimiter (otherwise hyphens could be problematic in YAML).
When it comes to deserializing these fields into language constructs, hyphen could be problematic in some languages as it might represent subtraction.
So I lean towards underscore.
There was a problem hiding this comment.
Let's clean it up and go with underscore!
There was a problem hiding this comment.
I filed #565 for this and will clean up the PR.
In elastic#552 `elasticsearch` as config block is introduced. As discussed there, it also makes sense to use this for the Ingest Pipeline config to have all Elasticsearch related parts in one place. For now no package uses this change and Kibana does not support it. As both ways are supported, this is not a breaking change yet. As a follow up, the package generation must be updated to adjust for this option and Kibana must be adapted to only support the new option.
|
@ycombinator PR updated, would be great if you could have a look again. |
| index_template.mappings: | ||
| dynamic: false | ||
| index_template.settings: | ||
| index.lifecycle.name: reference |
There was a problem hiding this comment.
Nit: you could break out the index_template key into it's own level and nest mappings and settings under it, like you did in testdata/package/yamlpipeline/1.0.0/dataset/log/manifest.yml.
There was a problem hiding this comment.
I actually didn't do it on purpose to make sure I test that both options work.
|
Left a nitpick. PR needs rebase due to conflicts. |
In some cases, the Elasticsearch index template for a dataset needs more configuration options then just the mappings that can be shipped as the fields.yml. To make this possible, this introduces the option to configure some settings in the dataset manifest. The naming is `elasticsearch.index-template` to be in line with the directory names. Inside settings and mappings are supported as free form. Anything else is ignored. This is not supported yet on the Kibana side. There this should either be put into a separate component template or merge on creation.
In elastic#552 `elasticsearch` as config block is introduced. As discussed there, it also makes sense to use this for the Ingest Pipeline config to have all Elasticsearch related parts in one place. For now no package uses this change and Kibana does not support it. As both ways are supported, this is not a breaking change yet. As a follow up, the package generation must be updated to adjust for this option and Kibana must be adapted to only support the new option.
In #552 `elasticsearch` as config block is introduced. As discussed there, it also makes sense to use this for the Ingest Pipeline config to have all Elasticsearch related parts in one place. For now no package uses this change and Kibana does not support it. As both ways are supported, this is not a breaking change yet. As a follow up, the package generation must be updated to adjust for this option and Kibana must be adapted to only support the new option.
In some cases, the Elasticsearch index template for a dataset needs more configuration options then just the mappings that can be shipped as the fields.yml. To make this possible, this introduces the option to configure some settings in the dataset manifest. The naming is
elasticsearch.index-templateto be in line with the directory names. Inside settings and mappings are supported as free form. Anything else is ignored.This is not supported yet on the Kibana side. There this should either be put into a separate component template or merge on creation.