Introduce elasticsearch.ingest_pipeline.name as config option#564
Introduce elasticsearch.ingest_pipeline.name as config option#564ruflin merged 3 commits intoelastic:masterfrom
elasticsearch.ingest_pipeline.name as config option#564Conversation
util/dataset.go
Outdated
| } | ||
|
|
||
| type Elasticsearch struct { | ||
| IngestPipelineName string `config:"ingest_pipeline.name,omitempty" json:"ingest_pipeline.name,omitempty" yaml:"ingest_pipeline.name,omitempty"` |
There was a problem hiding this comment.
@ycombinator Not 100% happy with the name yet. Any input?
There was a problem hiding this comment.
I think just IngestPipeline is enough? Even looking at Filebeat fileset's manifest.ymls, I don't think we've ever needed to define any sub-properties of ingest pipelines?
There was a problem hiding this comment.
There are 2 properties for ingest pipeline I had in mind:
- Path to the file locally
- Name defined in the manifest instead of generated name in the package manager
That is why I made it an object to leave this door open.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
|
| * Add list of downloads to /search endpoint. [#512](https://github.com/elastic/package-registry/pull/512) | ||
| * Apply rule: first package found served. [#546](https://github.com/elastic/package-registry/pull/546) | ||
| * Implement package watcher. [#553](https://github.com/elastic/package-registry/pull/553) | ||
| * Introduce `elasticsearch.ingest_pipeline.name` as config option. [#](https://github.com/elastic/package-registry/pull/) |
There was a problem hiding this comment.
Maybe add a note to the Deprecated section of the changelog about deprecating ingest_pipeline in favor of elasticsearch.ingest_pipeline?
There was a problem hiding this comment.
I'm guessing this (my previous comment) is also not needed as the old way is going away very soon?
| } | ||
|
|
||
| if d.IngestPipeline == "" { | ||
| if d.Elasticsearch != nil && d.Elasticsearch.IngestPipelineName == "" { |
There was a problem hiding this comment.
There is duplication of code inside this if block and it's sibling else if block. Looks like the intent is to prefer d.Elasticsearch.IngestPipelineName over d.IngestPipeline. How about defining a helper method, something like:
func (d *DataSet) ingestPipeline() string {
if d.Elasticsearch != nil && d.Elasticsearch.IngestPipelineName != "" {
return d.Elasticsearch.IngestPipelineName
}
return d.IngestPipeline
}
Then calling it here as using it's return value in the condition of the if. Then you should be able to get rid of the duplication. You could also save the return value in a local variable and use that further down wherever d.IngestPipeline is being used.
There was a problem hiding this comment.
As the code goes away very soon, I kept it "delete friendly". Do you think it is still worth cleaning up?
There was a problem hiding this comment.
No, I was not aware it was going away soon when I made the clean up comment. Okay to leave it as-is.
|
PR needs rebase due to conflicts. |
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.
c50804e to
783ad92
Compare
|
@ycombinator could you have a look at my updates and comments? |
In #552
elasticsearchas 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.