-
Notifications
You must be signed in to change notification settings - Fork 127
Set pipeline name automatically in transform definitions #2973
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
base: main
Are you sure you want to change the base?
Conversation
Allow to set an IngestPipelineName function in these templates in order to set the right ingest template filename. This filename must contain as a prefix the package version (from the manifest).
/test |
6 similar comments
/test |
/test |
/test |
/test |
/test |
/test |
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, some nitpicking and wondering if we should use text/template
instead of html/template
.
internal/packages/packages.go
Outdated
"encoding/json" | ||
"errors" | ||
"fmt" | ||
"html/template" |
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.
Is there any reason to use html/template
instead of text/template
? The html
one escapes some characters for safe HTML rendering, but we probably don't want or need this here.
"html/template" | |
"text/template" |
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.
No specific reason, I'll test with text/template
👍
internal/packages/packages.go
Outdated
|
||
t := template.New(filepath.Base(transformPath)) | ||
t, err = t.Funcs(template.FuncMap{ | ||
"IngestPipelineName": func(pipelineName string) (string, error) { |
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.
Nit. Looking to examples in the doc I think we should use lower case for functions.
"IngestPipelineName": func(pipelineName string) (string, error) { | |
"ingestPipelineName": func(pipelineName string) (string, error) { |
_, err = os.Stat(filepath.Join(packageRootPath, "elasticsearch", "ingest_pipeline", pipelineFileName)) | ||
if err != nil { | ||
return nil, TransformDefinition{}, fmt.Errorf("destination ingest pipeline file %s not found: %w", pipelineFileName, err) | ||
} |
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.
Did you test with integrations? Maybe some package fails with this 😬 Though if it does we should fix the package.
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.
Testing to build locally all packages in the integrations repository, there is one package that sets as dest.pipeline
the ingest pipeline from the data stream:
dest:
index: "aws_billing.cur-v1"
pipeline: "metrics-aws_billing.cur-0.1.0-pipeline_extract_metadata"
aliases:
- alias: "aws_billing.cur_latest"
move_on_creation: true
That fails with the current validation 🤔
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.
That package (aws_billing
) is failing this validation because of:
- It's using a pipeline from a data stream
- It set an old version in the destination pipeline, it should be 0.2.0.
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.
That pipeline from cur
data stream looks like it is not used:
https://github.com/elastic/integrations/blob/34df8c08914a678dc3310bd000e7908a1af42ef6/packages/aws_billing/data_stream/cur/elasticsearch/ingest_pipeline/pipeline_extract_metadata.yml
At least from the default pipeline there is no pipeline
processor to trigger that other pipeline:
https://github.com/elastic/integrations/blob/34df8c08914a678dc3310bd000e7908a1af42ef6/packages/aws_billing/data_stream/cur/elasticsearch/ingest_pipeline/default.yml
Here I've added the support to detect if the ingest pipeline exists on any data stream:
4f2756e
Probably, it could be moved the pipeline in that package to be on the elasticsearch
folder in the root, but in any case it could be interesting to keep the support to check if pipelines are located in data streams.
WDYT ?
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#15594 |
💚 Build Succeeded
History
cc @mrodm |
Fixes elastic/package-spec#833
This PR adds support to set automatically the pipeline name used in transforms adding the current package version as a prefix.
In order to achieve that
elasticsearch/transform/*/transform.yml
is managed byelastic-package
as a template file (Golang html template). And it is allowed to use a function in the template namedIngestPipelineName
that adds the current version of the package to the pipeline.Example:
This rendering of the template happens at different stages to ensure always is performed:
elastic-package build
elastic-package install
elastic-package test system
Author's checklist
How to test this PR locally