Allow to override attributes from external fields#575
Allow to override attributes from external fields#575jsoriano merged 3 commits intoelastic:masterfrom
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
| @@ -145,8 +145,12 @@ func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.Map | |||
| } | |||
|
|
|||
| transformed := transformImportedField(imported) | |||
There was a problem hiding this comment.
I'm wondering if it isn't possible to sketch the implementation without involving the DeepUpdate method (less error-prone code).
For example:
base := def
base.Delete("external")
transformed := transformImportedField(base, imported)
Function transformImportedField can create new base struct if it's nil.
There was a problem hiding this comment.
I will give a try, but this code would modify the definition, and I wanted to avoid this. For that we need a way to clone it and use as the base struct, or to deep update the final map.
Would you prefer clone over deep update?
There was a problem hiding this comment.
I just wouldn't like to add more potentially error-prone methods to mapStr, but if you feel confident the function maybe it's worth leaving it as is.
There was a problem hiding this comment.
Well, I copied it from libbeat where it has served well for years 🙂
Maybe in the future we can refactor these methods to use FieldDefinitions instead of mapStrs.
There was a problem hiding this comment.
I remember that Stefen strongly recommended not to use mapStr anymore :)
Anyway, I see that you covered this part with tests, so it shouldn't be a big unknown. Let's not elongate the discussion.
internal/docs/exported_fields.go
Outdated
| if err != nil { | ||
| return nil, errors.Wrap(err, "can't import field") | ||
| } | ||
| f.Type = imported.Type |
There was a problem hiding this comment.
nit: WDYT about introducing a new method to extract these if-conditions? For example: f.Update(imported)
There was a problem hiding this comment.
Done in 095d0ca.
Though doing it as a method of the FieldDefinition implies to do it for all its attributes, including the sub-fields. I have implemented this part by updating sub-fields with the same name, and appending the rest.
An alternative could have been to extract these if-conditions in a method here, not as a method of the struct, and then I could only update the fields relevant here.
Let me know what you think.
| // via DeepUpdate. | ||
| // DeepUpdateNoOverwrite is a version of this function that does not | ||
| // overwrite existing values. | ||
| func (m MapStr) DeepUpdate(d MapStr) { |
There was a problem hiding this comment.
I wish the DeepUpdate isn't mandatory in this implementation :)
| } | ||
|
|
||
| func (orig *FieldDefinition) Update(fd FieldDefinition) { | ||
| if fd.Name != "" { |
There was a problem hiding this comment.
Ok, so just have to remember about updating these conditions when adding a new property to the FieldDefinition.
I'm curious if it isn't shorter to use the YAML marshaller to update fields.
There was a problem hiding this comment.
Ok, so just have to remember about updating these conditions when adding a new property to the
FieldDefinition.
Yes, but well, having it close to the struct I guess that this is easier.
I'm curious if it isn't shorter to use the YAML marshaller to update fields.
I can give a try to this if prefered. Not sure if we will have flexibility on the nested fields, but we could keep this part as is.
There was a problem hiding this comment.
I don't want to slow you down with this as this is a working implementation. We can come back to this in the future.
Attributes defined in the fields definition are combined with the imported field, with some exceptions:
externalis removed, as it doesn't make sense in the final definition.typeis always used as defined in the imported field, to avoid having ECS fields with different types.Some use cases:
constant_keyword(Allow adding attributes to external field definitions #392).Fixes #392
Fixes elastic/package-spec#247