-
Notifications
You must be signed in to change notification settings - Fork 175
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
Feature: Backwards Compatible Agent TCP/UDP Socket #721
Conversation
…ensu/sensu-go into feature/back-compatible-socket
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
…ensu/sensu-go into feature/back-compatible-socket
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
…ensu/sensu-go into feature/back-compatible-socket
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
DCO sign-off is missing from earlier commits of this PR, before the contributing guideline was in place. |
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
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.
I'd prefer to keep uses of reflect in the dynamic package.
We should have lots of tests wherever reflect is invoked. translateToEvent should be exercised heavily by unit tests.
I don't think the use of reflection is justified or needed here. We can solve this problem by defining a 1.x legacy type, unmarshaling into it, and calling a method that converts the type to a 2.x type.
I think this would incur less technical debt, and would also have less risk of bugs. Bonus, we also eliminate the dependency on mapstructure for this package.
configVal := reflect.Indirect(reflect.ValueOf(check.Config)) | ||
checkVal := reflect.Indirect(reflect.ValueOf(event.Check)) | ||
for mapKey := range payload { | ||
_, existsInConfig := configVal.Type().FieldByName(strings.Title(mapKey)) |
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.
This does not work in all cases. There is no guarantee that the title-cased version of the json tag will match the struct field name.
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.
Would making the input string all lowercase, then capitalizing the first letter be sufficient: strings.Title(strings.ToLower(mapKey))
? Since all our struct fields have that format.
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.
No, that still won't work unfortunately. Consider this example:
type IsAProblem struct {
MyNameIs string `json:"slim_shady"`
}
I had to address this problem in some of the package dynamic code. You'd have to parse the JSON tag of the struct field and map it to the value in the map[string]interface{} map. See https://github.com/sensu/sensu-go/blob/master/types/dynamic/dynamic.go#L261
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.
Also, while all our types may have that convention right now, we can't let the implementation of a helper function dictate how we specify our types. We need to be able to change the field names, or have weirdo json tags, if necessary.
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.
Oh I see.. let me try to replace that solution!
Signed-off-by: Nikki Attea <nikki@sensu.io>
…ensu/sensu-go into feature/back-compatible-socket
Signed-off-by: Nikki Attea <nikki@sensu.io>
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.
…ensu/sensu-go into feature/back-compatible-socket
…ensu/sensu-go into feature/back-compatible-socket
…ensu/sensu-go into feature/back-compatible-socket Signed-off-by: Nikki Attea <nikki@sensu.io>
…ensu/sensu-go into feature/back-compatible-socket
Signed-off-by: Nikki Attea <nikki@sensu.io>
As per discussion, its probably safer to implement a v1 type here. Closing this out. |
What is this change?
Changes the TCP and UDP sockets to expect a 1.x compatible payload map[string]interface{} rather than a 2.x compatible event payload.
Why is this change necessary?
Closes #417.
Do you need clarification on anything?
Nah.
Were there any complications while making this change?
Nah.
Is this a new and complete feature?
Yup!