Added UDPOPtionSetters: ClientNet and ClientAddr. Setters bug fixed. … #42
Conversation
…Added tests. Added tests for UDPHook.Fire() method. Need fixing tests TestHooks and TestTrace.
| } | ||
| // ClientAddr sets clientAddr option in UDPHook | ||
| // Usage: hook, err := NewUDPHook(ClientAddr("192.168.1.1:65000")) | ||
| func ClientAddr(clientAddr string) UDPOptionSetter { |
There was a problem hiding this comment.
overall makes sense, one request though - instead of two functional argument parameters, let's make it one:
func Target(network, addr string) UDPOptionSetter {
return func(f *UDPHook) {
f.targetNet = network
f.targetAddr = addr
}
}There was a problem hiding this comment.
Yes, that makes sense, simplifying calls. I can add it. But I think what if somebody needs to set only one option? Or later we need to add 3rd option? I guess setting options one-by-one forces more writing, but makes things more flexible. One can set options only he/she needs and in any order. What do you think of it?
There was a problem hiding this comment.
we can address those problems later, I don't think this is a big deal for now
There was a problem hiding this comment.
I've added Target to set them at one call.
udphook_test.go
Outdated
| hook := UDPHook{} | ||
| f(&hook) | ||
|
|
||
| if hook.clientNet!= tt.want /*!reflect.DeepEqual(got, tt.want) */{ |
There was a problem hiding this comment.
My fail. Thank you for your attention.
udphook_test.go
Outdated
| hook := UDPHook{} | ||
| f(&hook) | ||
|
|
||
| if hook.clientAddr != tt.want /*!reflect.DeepEqual(got, tt.want) */{ |
- removed unnecessary types - removed leftover comments
…t conflicts with reserved fiedlds in Elastic.
| @@ -89,7 +88,6 @@ func (elk *UDPHook) Fire(e *log.Entry) error { | |||
| } | |||
| data, err := json.Marshal(Frame{ | |||
| Time: elk.Clock.Now().UTC(), | |||
| Type: "trace", | |||
There was a problem hiding this comment.
Field Type removed as it a reserved word and causes conflicts in Elastic
| @@ -72,7 +72,6 @@ type UDPHook struct { | |||
|
|
|||
| type Frame struct { | |||
| Time time.Time `json:"time"` | |||
| Type string `json:"type"` | |||
There was a problem hiding this comment.
Field Type removed as it a reserved word and causes conflicts in Elastic
…<autogenerated>", skip it.
I've added
UDPOptionSettersto set clientNet and clientAddr with tests.Also I've added
TestUDPHook_Fireto ensure all settings are applied correctly and system can work.During this I've found that real net and addr were hardcoded in line 94:
When I change it to required settins tests TestHook and TestTrace start to produce enormouse number of error messages. But tests themselves are correct.
Looks like in some another place there's a code relying on the hardcode above. Could you check it? Thank you.