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

add udp reachable check #545

Merged
merged 4 commits into from Feb 17, 2020

Conversation

@weakiwi
Copy link
Contributor

weakiwi commented Feb 11, 2020

add integration test for udp

Checklist
  • make test-all (UNIX) passes. CI will also test this
  • unit and/or integration tests are included (if applicable)

Description of change

Deng Yi
add integration test for udp
@weakiwi

This comment has been minimized.

Copy link
Contributor Author

weakiwi commented Feb 11, 2020

Related to #542

Copy link
Contributor

pedroMMM left a comment

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.

Copy link
Owner

aelsabbahy left a comment

Looks great, requesting a minor refactor.

Thanks for the contribution.

system/addr.go Show resolved Hide resolved
@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Feb 12, 2020

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

Deng Yi
@weakiwi weakiwi requested a review from aelsabbahy Feb 13, 2020
@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Feb 13, 2020

@weakliwi I'll review and merge this sometime tomorrow.

Thank you for your contribution!

@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Feb 16, 2020

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.

@weakiwi

This comment has been minimized.

Copy link
Contributor Author

weakiwi commented Feb 16, 2020

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

@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Feb 17, 2020

Looks great, thanks!

@aelsabbahy aelsabbahy merged commit 81d5f15 into aelsabbahy:master Feb 17, 2020
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Feb 17, 2020

@pedroMMM I was wrong, it didn't auto mark #542 as closed. Closing it manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.