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

Remove unexpected route from route list #843

Merged
merged 1 commit into from
Oct 31, 2017
Merged

Conversation

chenchun
Copy link
Contributor

No description provided.

@tomdee
Copy link
Contributor

tomdee commented Oct 20, 2017

thanks @chenchun this looks like a good fix. Before I merge it, can you share a little bit of information on how you hit this and how you've tested the fix. Ideally there would be an automated test case for this too, would that be possible to add?

@chenchun chenchun force-pushed the remove branch 2 times, most recently from 1dc26f8 to dda8a12 Compare October 24, 2017 11:56
@chenchun
Copy link
Contributor Author

Found this bug through reading the code. I've added an unit test. PTAL

@tomdee
Copy link
Contributor

tomdee commented Oct 28, 2017

Looks good thanks. You'll need to update the Makefile too though, the TEST_PACKAGES variable needs to include backend/hostgw

@chenchun chenchun force-pushed the remove branch 5 times, most recently from 698c9d5 to 9e2a306 Compare October 30, 2017 05:05
@chenchun
Copy link
Contributor Author

chenchun commented Oct 30, 2017

Previous tests failed with

--- FAIL: TestRouteCache (0.00s)
	hostgw_network_test.go:43: operation not permitted

Updated Makefile to makes use of docker run to give go test NET_ADMIN capability.

@tomdee
Copy link
Contributor

tomdee commented Oct 31, 2017

This does mean that Docker is needed for running the unit tests, but I think that's OK now since the end to end tests need Docker too.

@tomdee tomdee merged commit 2bc97a5 into flannel-io:master Oct 31, 2017
@chenchun chenchun deleted the remove branch November 1, 2017 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants