search

tynany / frr_exporter

Prometheus exporter for Free Range Routing
MIT License
100 stars 34 forks source link

fix: ExecuteCmd handling of output larger than buffer size #84

Closed daveset closed 2 years ago

daveset commented 2 years ago

Fixes Issue #83

tynany commented 2 years ago

Thank you!

@dswarbrick do you want to review this?

dswarbrick commented 2 years ago

The fix in frrsockets.go LGTM.

However I'm not a fan of generating the JSON for the test on the fly. That's really what test fixtures are for, i.e. include some pre-generated JSON (or even better, some actual JSON from FRR) in a testdata directory, or as a raw string in frr_sockets_test.go. Also, whether the socket is receiving JSON or plain text is not relevant to the test.

daveset commented 2 years ago

I get your point re generating json. I wasn't sure if there was anything in our peering data I wouldn't want to expose, so I chose to generate it. As you point out it is totally arbitrary mocked data so any string longer than 4096 would trigger the issue.

I've updated to generate a random string of data of length 5000 and also slightly refactored to put the socket handling in a function used by both tests.

If you would prefer something different in terms of using testdata fixtures or the like I can do that but I can't promise when I will be able to get to it.

dswarbrick commented 2 years ago

LGTM

tynany commented 2 years ago

Thank you, @daveset & @dswarbrick !