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

Fix parsing on older versions of salt #69

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

grimmy
Copy link
Contributor

@grimmy grimmy commented Aug 21, 2024

On older versions of salt, the message body is already a string which causes the .([]byte) type assertion to fail. This catches that failure than then tries an .(string) assertion to handle both cases.

Fixes #68

On older versions of salt, the message body is already a string which causes
the .([]byte) type assertion to fail. This catches that failure than then
tries an .(string) assertion to handle both cases.
if raw, ok := message["body"].([]byte); ok {
body = string(raw)
} else {
body = message["body"].(string)
Copy link
Owner

Choose a reason for hiding this comment

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

could you please add a small comment to explain the reason of doing this fallback

body := string(message["body"].([]byte))
var body string

if raw, ok := message["body"].([]byte); ok {
Copy link
Owner

@kpetremann kpetremann Aug 22, 2024

Choose a reason for hiding this comment

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

opt/nit: using a type switch may be cleaner

@kpetremann
Copy link
Owner

kpetremann commented Aug 22, 2024

hello,

Thanks for the PR :)

I would have preferred to reproduce the issue first with Salt old version.
I am not sure whether the issue is coming from old Salt or old MsgPack. And I would like to make sure it won't break elsewhere for the old version.

@grimmy
Copy link
Contributor Author

grimmy commented Aug 22, 2024

hello,

Thanks for the PR :)

Of course!

I would have preferred to reproduce the issue first with Salt old version. I am not sure whether the issue is coming from old Salt or old MsgPack. And I would like to make sure it won't break elsewhere for the old version.

I can reliably reproduce here as we have an old production server that we need to migrate from, but in the mean time I'm trying to add some monitoring before we do anything which led to me finding the issue.

@kpetremann kpetremann merged commit 1d5958d into kpetremann:main Aug 23, 2024
3 checks passed
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.

type assertion failure right after startup
2 participants