-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Make conditions in autodiscovery configs optional #9029
Conversation
0e157c2
to
210f6a6
Compare
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 for the change, this is looking good! Left a minor comment. I think this will also need an update in docs?
@@ -42,20 +42,20 @@ type MapperSettings []*struct { | |||
} | |||
|
|||
// NewConfigMapper builds a template Mapper from given settings | |||
func NewConfigMapper(configs MapperSettings) (*Mapper, error) { | |||
var mapper Mapper | |||
func NewConfigMapper(configs MapperSettings) (mapper Mapper, err 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.
are you using the named return values? I think that's not needed from 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.
It prevents me having to declare mapper
on line 55 in the call to append.
This would close #6084, as it would make it trivial to add a last "conditionless" config. Added the |
210f6a6
to
b7e916a
Compare
@exekias I've rebased and added a small change to the docs. We never really documented conditions before, I don't think we need to explicitly say much 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.
CHANGELOG needs a line removed I think, feel free to fix it and merge
Uhm, I just saw tests are not passing, it seems related to this change |
This change lets users leave the `condition` option undeclared when writing autodiscover configurations. When not declared all configurations will match all events.
d8b0235
to
a5cf04c
Compare
@exekias I believe I've fixed the issue causing the tests to fail. I've also rebased off master. |
* Make conditions in autodiscovery configs optional This change lets users leave the `condition` option undeclared when writing autodiscover configurations. When not declared all configurations will match all events. * Cleanup * Fix fmt * Fix changelog * Fix failing conditions test (cherry picked from commit a1bec73)
This change lets users leave the
condition
option undeclared when writing autodiscover configurations. When not declared all configurations will match all events.This also changes the type of the autodiscovery
Mapper
from*Mapper
toMapper
. Since this type is backed by a slice, which is internally a pointer, there's no need to make it a pointer type, as it it makes the append operation awkward. If we internally mutated the slice via method's it'd make sense to keep the pointer, but we don't.This takes over from #8897 .