-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add templating support to pipeline processor. #49030
Add templating support to pipeline processor. #49030
Conversation
This commit adds templating support to the pipeline processor's `name` option. Closes elastic#39955
Pinging @elastic/es-core-features (:Core/Features/Ingest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great @martijnvg
Left a few minor suggestions and a question on the docs
@@ -7,7 +7,7 @@ Executes another pipeline. | |||
[options="header"] | |||
|====== | |||
| Name | Required | Default | Description | |||
| `name` | yes | - | The name of the pipeline to execute | |||
| `name` | yes | - | The name of the pipeline to execute. Supports <<accessing-template-fields,template snippets>>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have these links changed? accessing-template-fields
points to common-options
and template snippets
to the accessing template fields
paragraph on the pipeline.asciidoc
page but there isn't such a paragraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a direct copy from the set processor docs page and that links correctly. So I think it is good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, might be the github navigation that's not able to follow the links correctly.
import java.util.Map; | ||
import java.util.function.BiConsumer; | ||
|
||
public class PipelineProcessor extends AbstractProcessor { | ||
|
||
public static final String TYPE = "pipeline"; | ||
|
||
private final String pipelineName; | ||
|
||
private final TemplateScript.Factory pipelineName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to rename this to something that points it to the templating aspect? (eg. pipelineTemplate
?) it's a bit confusing otherwise below when we render the template and yield a pipelineName
from another pipelineName
@@ -61,7 +64,7 @@ public String getType() { | |||
return TYPE; | |||
} | |||
|
|||
String getPipelineName() { | |||
TemplateScript.Factory getPipelineName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would avoid a bit of confusion if the getter is renamed to getPipelineNameTemplate
or something along those lines (it took me a while to understand we are not instantiating a name here 91d8ada#diff-579dffc1e22e3db13c41f685046b2891R443)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM nicely done @martijnvg
This commit adds templating support to the pipeline processor's `name` option. Closes elastic#39955
This commit adds templating support to the pipeline processor's
name
option.Closes #39955