-
Notifications
You must be signed in to change notification settings - Fork 117
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
Import ecs fields from the ecs_nested.yml source file #766
Conversation
@@ -1,6 +1,4 @@ | |||
- name: destination.geo.location | |||
external: ecs | |||
- name: geo.location | |||
external: ecs |
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 field is actually invalid, as reported in #750.
/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, but to be honest we should get rid of this script as soon as possible. I don't know what is the percentage of migration completeness.
@mtojek take into account that this is for the fields imported for the The mention to beats is because before this change we are using the ecs fields document generated for beats (like this one), and after this change we would be using one of the generic ones (like this one). This way we can import anything from the ECS definitions, and not only what is relevant to Beats. I guess we don't want to get rid of |
internal/fields/model.go
Outdated
case yaml.MappingNode: | ||
// Fields are defined as a map, this happens in ecs fields files. | ||
var fields []FieldDefinition | ||
for i := 0; i < len(value.Content); i += 2 { |
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 it safe to assume it's always a pair? Is there a chance that it will become a triple :)? a comment, or second value...
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.
In principle yes, the content of a mapping node should be pairs of key/values. Comments are included in the significative nodes themselves.
But let me add an additional constraint here to avoid reading the last i+1
value when it doesn't exist.
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.
Added in c79194b.
Import ECS fields from
ecs/ecs_nested.yml
instead of frombeats/fields.ecs.yml
.Beats fields don't include all the information provided by ECS, for example they don't include the
normalize
rules, required for #615. I guess this can be expected, as there can be features of ECS that are not used or needed in Beats, but could be used by other tools.ecs_nested.yml
takes into account where nested objects can be reused. This fixes #750.