-
Notifications
You must be signed in to change notification settings - Fork 471
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
add udp reachable check #545
add udp reachable check #545
Conversation
add integration test for udp
Related to #542 |
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.
Simple and to the point, great!
The only feedback I have is that you forgot to add something like fix #542
in the PR Description like the documentation that is linked on the PR Template askes.
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.
Looks great, requesting a minor refactor.
Thanks for the contribution.
For what it's worth, I think the whole fixes #542 works in the comments too. I'll see if this comment ends up closing it when the ticket is merged |
@weakliwi I'll review and merge this sometime tomorrow. Thank you for your contribution! |
Sorry for running late on the merge of this. As long as there are no merge conflicts you don't have to keep merging or rebasing master. I'll try to get this done tomorrow or first thing Monday. |
OK. Get it |
Looks great, thanks! |
* add udp reachable check add integration test for udp * refactor for how variable creating
add integration test for udp
Checklist
make test-all
(UNIX) passes. CI will also test thisDescription of change