Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for condition on bool type #5954

Merged
merged 6 commits into from Jan 25, 2018
Merged

Add support for condition on bool type #5954

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ca4dd2
Add support for condition on bool type (#5659)
ewgRa Dec 23, 2017
5af5ad6
Merge branch 'master' into equals-bool-type-5659
ewgRa Jan 14, 2018
665a1be
fix tests
ewgRa Jan 14, 2018
7645f81
less nested if/else
ewgRa Jan 14, 2018
e4512aa
Merge branch 'master' into equals-bool-type-5659
ewgRa Jan 22, 2018
72c7eec
review fixes: nl, self error and Err instead Warn
ewgRa Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Expand Up @@ -177,6 +177,7 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
*Packetbeat*

- Configure good defaults for `add_kubernetes_metadata`. {pull}5707[5707]
- Add support for condition on bool type {issue}5659[5659] {pull}5954[5954]

*Winlogbeat*

Expand Down
50 changes: 38 additions & 12 deletions libbeat/processors/condition.go
View file
Open in desktop
Expand Up @@ -21,8 +21,9 @@ type RangeValue struct {
}

type EqualsValue struct {
Int uint64
Str string
Int uint64
Str string
Bool bool
}

type Condition struct {
Expand Down Expand Up @@ -118,13 +119,22 @@ func (c *Condition) setEquals(cfg *ConditionFields) error {
uintValue, err := extractInt(value)
if err == nil {
c.equals[field] = EqualsValue{Int: uintValue}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should use continue in the code to have less nested if / else? Same on line 125.

} else {
sValue, err := extractString(value)
if err != nil {
return err
}
continue
}

sValue, err := extractString(value)
if err == nil {
c.equals[field] = EqualsValue{Str: sValue}
continue
}

bValue, err := extractBool(value)
if err == nil {
c.equals[field] = EqualsValue{Bool: bValue}
continue
}

return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should create our "own" error here to describe better what happened. Otherwise just return the err from extractBool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Have the same thought, just don't know how far changes can be.

}

return nil
Expand Down Expand Up @@ -257,16 +267,32 @@ func (c *Condition) checkEquals(event ValuesMap) bool {
if intValue != equalValue.Int {
return false
}
} else {
sValue, err := extractString(value)
if err != nil {
logp.Warn("unexpected type %T in equals condition as it accepts only integers and strings. ", value)

continue
}

sValue, err := extractString(value)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

if err == nil {
if sValue != equalValue.Str {
return false
}
if sValue != equalValue.Str {

continue
}

bValue, err := extractBool(value)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline

if err == nil {
if bValue != equalValue.Bool {
return false
}

continue
}

logp.Warn("unexpected type %T in equals condition as it accepts only integers, strings or bools. ", value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to logp.Err? In general we prefer not to use Warn and make things either Info or Err.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return false
}

return true
Expand Down
19 changes: 18 additions & 1 deletion libbeat/processors/condition_test.go
View file
Open in desktop
Expand Up @@ -129,6 +129,22 @@ func TestCondition(t *testing.T) {
},
result: true,
},
{
config: ConditionConfig{
Equals: &ConditionFields{fields: map[string]interface{}{
"final": true,
}},
},
result: false,
},
{
config: ConditionConfig{
Equals: &ConditionFields{fields: map[string]interface{}{
"final": false,
}},
},
result: true,
},
}

event := &beat.Event{
Expand All @@ -150,7 +166,8 @@ func TestCondition(t *testing.T) {
"username": "monica",
"keywords": []string{"foo", "bar"},
},
"type": "process",
"type": "process",
"final": false,
},
}

Expand Down
9 changes: 9 additions & 0 deletions libbeat/processors/config.go
View file
Open in desktop
Expand Up @@ -130,3 +130,12 @@ func extractString(unk interface{}) (string, error) {
return "", fmt.Errorf("unknown type %T passed to extractString", unk)
}
}

func extractBool(unk interface{}) (bool, error) {
switch b := unk.(type) {
case bool:
return b, nil
default:
return false, fmt.Errorf("unknown type %T passed to extractBool", unk)
}
}
10 changes: 10 additions & 0 deletions libbeat/processors/config_test.go
View file
Open in desktop
Expand Up @@ -15,3 +15,13 @@ func TestExtractString(t *testing.T) {
}
assert.Equal(t, input, v)
}

func TestExtractBool(t *testing.T) {
input := true

v, err := extractBool(input)
if err != nil {
t.Fatal(err)
}
assert.Equal(t, input, v)
}