Skip to content
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

Introduce elasticsearch.ingest_pipeline.name as config option #564

Merged
merged 3 commits into from
Jul 2, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* 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)
* Add conditions as future replacement of requirements. [#519](https://github.com/elastic/package-registry/pull/519)
* Introduce `elasticsearch.ingest_pipeline.name` as config option. [#](https://github.com/elastic/package-registry/pull/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note to the Deprecated section of the changelog about deprecating ingest_pipeline in favor of elasticsearch.ingest_pipeline?

Copy link
Contributor

@ycombinator ycombinator Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this (my previous comment) is also not needed as the old way is going away very soon?


### Deprecated

28 changes: 23 additions & 5 deletions util/dataset.go
Original file line number Diff line number Diff line change
@@ -22,6 +22,10 @@ import (

const (
DirIngestPipeline = "ingest-pipeline"

DefaultPipelineName = "default"
DefaultPipelineNameJSON = "default.json"
DefaultPipelineNameYAML = "default.yaml"
)

var validTypes = map[string]string{
@@ -34,8 +38,10 @@ type Dataset struct {
Type string `config:"type" json:"type" validate:"required"`
Name string `config:"name" json:"name,omitempty" yaml:"name,omitempty"`

Title string `config:"title" json:"title" validate:"required"`
Release string `config:"release" json:"release"`
Title string `config:"title" json:"title" validate:"required"`
Release string `config:"release" json:"release"`

// Deprecated: Replaced by elasticsearch.ingest_pipeline.name
IngestPipeline string `config:"ingest_pipeline,omitempty" config:"ingest_pipeline" json:"ingest_pipeline,omitempty" yaml:"ingest_pipeline,omitempty"`
ruflin marked this conversation as resolved.
Show resolved Hide resolved
Streams []Stream `config:"streams" json:"streams,omitempty" yaml:"streams,omitempty" `
Package string `json:"package,omitempty" yaml:"package,omitempty"`
@@ -81,6 +87,7 @@ type Variable struct {
type Elasticsearch struct {
IndexTemplateSettings map[string]interface{} `config:"index_template.settings" json:"index_template.settings,omitempty" yaml:"index_template.settings,omitempty"`
IndexTemplateMappings map[string]interface{} `config:"index_template.mappings" json:"index_template.mappings,omitempty" yaml:"index_template.mappings,omitempty"`
IngestPipelineName string `config:"ingest_pipeline.name,omitempty" json:"ingest_pipeline.name,omitempty" yaml:"ingest_pipeline.name,omitempty"`
}

type fieldEntry struct {
@@ -157,11 +164,22 @@ func (d *Dataset) Validate() error {
return fmt.Errorf("type is not valid: %s", d.Type)
}

if d.IngestPipeline == "" {
if d.Elasticsearch != nil && d.Elasticsearch.IngestPipelineName == "" {
Copy link
Contributor

@ycombinator ycombinator Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the code goes away very soon, I kept it "delete friendly". Do you think it is still worth cleaning up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I was not aware it was going away soon when I made the clean up comment. Okay to leave it as-is.

// Check that no ingest pipeline exists in the directory except default
for _, path := range paths {
if filepath.Base(path) == DefaultPipelineNameJSON || filepath.Base(path) == DefaultPipelineNameYAML {
d.Elasticsearch.IngestPipelineName = DefaultPipelineName
// TODO: remove because of legacy
d.IngestPipeline = DefaultPipelineName
break
}
}
// TODO: Remove, only here for legacy
} else if d.IngestPipeline == "" {
// Check that no ingest pipeline exists in the directory except default
for _, path := range paths {
if filepath.Base(path) == "default.json" || filepath.Base(path) == "default.yml" {
d.IngestPipeline = "default"
if filepath.Base(path) == DefaultPipelineNameJSON || filepath.Base(path) == DefaultPipelineNameYAML {
d.IngestPipeline = DefaultPipelineName
break
}
}