-
Notifications
You must be signed in to change notification settings - Fork 88
Handlebars template static validation and documentation #1030
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
Handlebars template static validation and documentation #1030
Conversation
|
test integrations |
| pkgEntries, err := fs.Glob(fsys, "agent/**/*.hbs") | ||
| if err != nil && !errors.Is(err, fs.ErrNotExist) { | ||
| return nil, 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.
According to the spec, this files could also be links.
Would this catch even the links thanks to the filesystem ? Or is it needed to add a special case for link files?
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've updated the PR to include the linked files. For this i've changed the aproach of glob and used nested readDirs to get into the files.
i've added tests and test packages to check this works as expected with the links 👍🏻
|
|
||
| // validateFile validates a single Handlebars file located at filePath. | ||
| // it parses the file using the raymond library to check for syntax errors. | ||
| func validateFile(filePath 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.
I'd rename this to be more specific, what about validateHandlebarTemplateFile or validateHandlebarTemplate?
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.
yup, i've gone around this one :D as the "parent" function is very similar... i will think around this
…bsolute path for linked files
|
test integrations |
|
Created or updated PR in integrations repository to test this version. Check elastic/integrations#16224 |
| // If fs.ReadFile fails (likely due to linked file path outside filesystem boundary), | ||
| // fall back to absolute path approach like linkedfiles.FS does | ||
| absolutePath := fsys.Path(filePath) | ||
| if content, err = os.ReadFile(absolutePath); err != nil { | ||
| return 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.
According to the comment, is this already done by the linkedfiles.FS ? If so, maybe it could be removed from here.
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 refers to the logic at getLinkedFileChecksum where the file content is read to get the checksum. If the file is not within fsys fs.ReadFile fails, as is not in its scope. If this happens, we use os.ReadFile (like in getLinkedFileChecksum) to read the file with the absolute path of it
| } | ||
| err = validateHandlebarsEntry(fsys, dir, linkFile.IncludedFilePath) | ||
| if err != nil { | ||
| return 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.
Could it be defined a specerrors.ValidationErrors here in this function and add all the errors found in a given folder?
I was thinking if it could be possible to show all the errors at once to the user.
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.
fixed this and adding more context to the error 👍🏻
an example of the output when testing one of the failed integration:
Error: building package failed: invalid content found in built zip package: found 1 validation error:
1. invalid handlebars template: error validating data_stream/alerts_events_v2/agent/stream/gcs.yml.hbs: Parse error on line 49:
Expecting OpenEndBlock, got: 'EOF'
… tests for clarity
|
after running the test against the current integrations, two of them had errors regarding some template parsing. |
| Config template file for inputs defined in the policy_templates section of the top level manifest. | ||
| The template should use standard Handlebars syntax (e.g., `{{vars.key}}`, `{{#if vars.condition}}`, `{{#each vars.items}}`) | ||
| and must compile to valid YAML. | ||
| Available Handlebars helpers include: |
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.
Can you please document the metadata variables that are available in templates too. See elastic/kibana#241140
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.
udpated the pr bd2358e with more docs on variables
…nd clarify helper list
💚 Build Succeeded
History
|
mrodm
left a comment
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, thanks!
jsoriano
left a comment
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.
Only a couple of comments, but I am fine with implemented approach.
| func TestValidateTemplateDir(t *testing.T) { | ||
| t.Run("empty directory", func(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| pkgDir := path.Join(tmpDir, "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.
Nit. Most paths in these tests are native paths, they should be joined with filepath.Join.
| {fn: semantic.ValidateMinimumAgentVersion}, | ||
| {fn: semantic.ValidateIntegrationPolicyTemplates, types: []string{"integration"}}, | ||
| {fn: semantic.ValidatePipelineTags, types: []string{"integration"}, since: semver.MustParse("3.6.0")}, | ||
| {fn: semantic.ValidateStaticHandlebarsFiles, types: []string{"integration", "input"}}, |
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 consider to place the validation in the spec? It could have been done by using a content media type and something like the validateContentType method.
But using a semantic validator LGTM too in any case.
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 not. Thanks for this suggestion, i see now that adding this to the spec, perhaps with a custom type application/x-hbs could simplify the validation, as it will detect the files, instead of looking for them under the folders as the current implementation does.... 🤔 although for linked files i am not sure how it will work, i guess we could rely on the internal linked file to exist, and this being validated as hbs where originally is placed.
i think this approach could be an improvement
What does this PR do?
Includes a validation rule for verifying the static
.hbsfiles used at input tempate and stream template are correct.Also adds more information on the spec regarding the format of these templates and the helpers that are available at fleet when rendering them.
Linked file are also verified
.hbs.linkThe validation is done over the static syntax of handlebars, does not validate the render of the template taking into account the values of the values.
Why is it important?
If a template arrives fleet without being checked, the can be errors when fleet renders the template. This is a way to prevent this before the package is installed.
Checklist
Added unit tests and validation test with test packages
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues
Resolves #21