-
Notifications
You must be signed in to change notification settings - Fork 129
Fix documentation of ECS fields in linked files #3104
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
Conversation
|
test integrations |
|
Created or updated PR in integrations repository to test this version. Check elastic/integrations#16238 |
| if err != nil { | ||
| return fmt.Errorf("locating repository root failed: %w", err) | ||
| } | ||
| defer repositoryRoot.Close() |
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.
There are more usages of files.FindRepositoryRoot() where it sohuld be added that defer repositoryRoot.Close():
- cmd/links.go
- cmd/install.go
- cmd/lint.go
- cmd/build.go
- ...
https://github.com/search?q=repo%3Aelastic%2Felastic-package%20FindRepositoryRoot&type=code
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.
- cmd/links.go
- cmd/install.go
- cmd/lint.go
- cmd/build.go
On these files the defer was already added, but reviewing it I have found that it was still missing in some test actions, fixed in 5c1205c.
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.
On these files the defer was already added
Ouch, just look for the string and assumed that there was no close in the others, sorry
| } | ||
| if includedPackageRoot != "" { | ||
| if err == nil && includedPackageRoot != "" { | ||
| // Linked file doesn't need to be in a package, so ignore failures looking for it. |
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.
👍
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
For double checking, I just thought about transform folders. Could you test that this would work also in transform fields files defined via links ?
Looking at the spec, those folders also allow link files there since they are using the same spec definition:
https://github.com/elastic/package-spec/blob/edad324120fc8fb1655f26ce0aedb10296074703/spec/integration/elasticsearch/transform/spec.yml#L55
At least those files are also listed as part of the listAllFieldsFiles:
elastic-package/internal/builder/external_fields.go
Lines 88 to 96 in ba41d7d
| func listAllFieldsFiles(packageRoot string) ([]string, error) { | |
| patterns := []string{ | |
| // Package fields | |
| filepath.Join(packageRoot, "fields", "*.yml"), | |
| // Data stream fields | |
| filepath.Join(packageRoot, "data_stream", "*", "fields", "*.yml"), | |
| // Transform fields | |
| filepath.Join(packageRoot, "elasticsearch", "transform", "*", "fields", "*.yml"), | |
| } |
elastic-package/internal/builder/external_fields.go
Lines 63 to 83 in ba41d7d
| for _, file := range fieldsFiles { | |
| data, err := os.ReadFile(file) | |
| if err != nil { | |
| return err | |
| } | |
| rel, _ := filepath.Rel(buildPackageRoot, file) | |
| output, injected, err := injectFields(fdm, data, options) | |
| if err != nil { | |
| return err | |
| } else if injected { | |
| logger.Debugf("%s: source file has been changed", rel) | |
| err = os.WriteFile(file, output, 0644) | |
| if err != nil { | |
| return err | |
| } | |
| } else { | |
| logger.Tracef("%s: source file hasn't been changed", rel) | |
| } | |
| } |
IIRC fields in transforms are not rendered in Readmes files (aren't they?). However, just to be sure that when using external: ecs in transforms fields, they get replaced in the build folder when those files are links.
| if err != nil { | ||
| return fmt.Errorf("locating repository root failed: %w", err) | ||
| } | ||
| defer repositoryRoot.Close() |
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.
On these files the defer was already added
Ouch, just look for the string and assumed that there was no close in the others, sorry
Adding linked files to transforms in test packages in 0e01d70 I have tested locally and they are correctly resolved.
This logic is in principle applied only to built packages, when the links are already resolved. |
💚 Build Succeeded
History
cc @jsoriano |
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.
Thanks!
Fields validator is used for many things, including looking for fields for documentation.
In any case, it needs to resolve linked fields files, so the fields the linked files contain are included in documentation, built packages and validation processes.
This change explicitly includes links resolution in the validator, what makes it depend on a repository root. Most of the changes here are related to passing the repository root to the validator.