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.

Sign up for GitHub

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

Introduce elasticsearch.ingest_pipeline.name as config option #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e9ccee
Introduce `elasticsearch.ingest_pipeline.name` as config option
ruflin Jun 29, 2020
b888df7
commit review
ruflin Jul 1, 2020
783ad92
merge object
ruflin Jul 1, 2020
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

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

const (
DirIngestPipeline = "ingest-pipeline"

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

var validTypes = map[string]string{
Expand All @@ -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"`
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 •
edited
Loading

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
}
}
Expand Down