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.

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

Closed
wants to merge 39 commits into from

Conversation

nikkictl
Copy link

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!

Nikki Attea added 28 commits December 7, 2017 15:28
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
@nikkictl
Copy link
Author

DCO sign-off is missing from earlier commits of this PR, before the contributing guideline was in place.

Nikki Attea added 2 commits December 11, 2017 19:59
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Copy link
Contributor

@echlebek echlebek left a 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))
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Author

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!

Nikki Attea added 3 commits December 12, 2017 01:01
Signed-off-by: Nikki Attea <nikki@sensu.io>
Signed-off-by: Nikki Attea <nikki@sensu.io>
Copy link
Contributor

@palourde palourde left a comment

Choose a reason for hiding this comment

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

Everything else looks good to me!

source

@nikkictl
Copy link
Author

As per discussion, its probably safer to implement a v1 type here. Closing this out.

@nikkictl nikkictl closed this Dec 13, 2017
@nikkictl nikkictl deleted the feature/back-compatible-socket branch December 15, 2017 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants