Add support for condition on bool type #5954
Conversation
|
Can one of the admins verify this patch? |
ewgRa
changed the title
ewgRa
changed the title
ewgRa
changed the title
| @@ -120,10 +121,17 @@ func (c *Condition) setEquals(cfg *ConditionFields) error { | |||
| c.equals[field] = EqualsValue{Int: uintValue} | |||
There was a problem hiding this comment.
Perhaps we should use continue in the code to have less nested if / else? Same on line 125.
libbeat/processors/condition.go
Outdated
| if sValue != equalValue.Str { | ||
| return false | ||
|
|
||
| if err == nil { |
There was a problem hiding this comment.
Same comment as above, use continue to make the code more readable and less nested.
libbeat/processors/condition_test.go
Outdated
| @@ -93,6 +93,18 @@ func TestEqualsCondition(t *testing.T) { | |||
| "proc.cpu.total_p.gt": 0.5, | |||
| }}, | |||
| }, | |||
|
|
|||
| { | |||
| Equals: &ConditionFields{fields: map[string]interface{}{ | |||
There was a problem hiding this comment.
I opened #5960 to make adding tests cleaner. I realised when reading this that the way tests are written at the moment is not optimal for diffs.
|
jenkins, test it |
This simplifies part of the condition tests to use table tests. Like this the expected result and the condition itself are close together in the code which makes it easier to read. Cleanup was triggered when looking at elastic#5954 and realised it hard to follow in a diff.
This simplifies part of the condition tests to use table tests. Like this the expected result and the condition itself are close together in the code which makes it easier to read. Cleanup was triggered when looking at #5954 and realised it hard to follow in a diff.
|
@ruflin done About "continue", for me both ways looks not so good, since "continue" also like "goto" here, but it is definitely better than X levels of if-else. I also thinking about extract this part to "extractValue" function, "NewEqualsValue(unk...)", or introduce "BoolCondition", but hard to say for me will be this better, or worse, so, I think current implementation also will work. Please review |
|
jenkins, test it |
libbeat/processors/condition.go
Outdated
| } | ||
|
|
||
| bValue, err := extractBool(value) | ||
|
|
libbeat/processors/condition.go
Outdated
| } | ||
|
|
||
| sValue, err := extractString(value) | ||
|
|
There was a problem hiding this comment.
Done in both cases.
Can you give explanations? Is there coding style agreements/convention and how it looks like?
I think would be nice to add this to "make fmt" for example, anyway, would be nice to give this work to some script. If you give me explanations, I will try to prepare PR that made such checks automatically as part of CI process.
There was a problem hiding this comment.
This is more a beats team convention that we normally check an error directly on the next line. Not sure if this can be covered with a script as it's pretty specific to the error case. Don't think too much about it.
There was a problem hiding this comment.
Is it possible to publish beats team convention in Dev Guide?
There is only one mention of coding style related to EditorConfig, but looks like there is another. I think it save a lot of time to you and contributors.
And perhaps it is possible to automate, with parser, or ast, we just need to check that it is "if" and in expression there is value with type "error", but I'm not sure, haven't so much experience with ast and parser.
There was a problem hiding this comment.
Ok, looks like for fixing coding style we can start looking for something similar as https://github.com/golang/go/tree/master/src/cmd/fix or gofix. I pretty sure there is a possibility to do desired. I think I will try later.
There was a problem hiding this comment.
@ewgRa All the code rules we require are covered by make fmt. The other ones like no new line to check the error and I'm sure there are a few others, are not very strict. We should add them to the Dev Guide but TBH we as a team would also have to think about which these actually are. It would be great if all of them could be checked by a tool
libbeat/processors/condition.go
Outdated
| continue | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
I wonder if we should create our "own" error here to describe better what happened. Otherwise just return the err from extractBool.
There was a problem hiding this comment.
Done
Have the same thought, just don't know how far changes can be.
libbeat/processors/condition.go
Outdated
| } | ||
|
|
||
| logp.Warn("unexpected type %T in equals condition as it accepts only integers, strings or bools. ", value) |
There was a problem hiding this comment.
Can we change this to logp.Err? In general we prefer not to use Warn and make things either Info or Err.
|
jenkins, test it |
Pull request that add ability to use bool in processor conditions, related to #5659