-
Notifications
You must be signed in to change notification settings - Fork 128
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
Moved IP Reconciler code into IP Control Loop #238
Moved IP Reconciler code into IP Control Loop #238
Conversation
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.
Nice. Mostly I just have nits.
c05b2bb
to
a1a106b
Compare
a1a106b
to
448f37b
Compare
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.
You'll need to move the shutdown of the work-queue to the main function - it must run after the stopChannel is caught.
I would simply defer it within the main. For that, you'll need to add a receiver function to the pod controller that shuts down the queue.
Signed-off-by: nicklesimba <[email protected]>
fdb5977
to
1ba8ea9
Compare
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.
You now registering the fail handler, and attempting to run the specs twice in the same pkg.
You cannot do that. Remove the following lines from the pkg/reconciler/suite_test.go
file.
func TestAPIs(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecsWithDefaultAndCustomReporters(t,
"Whereabouts IP reconciler Suite",
[]Reporter{})
}
Or, better yet, merge the pkg/reconciler/ip_test.go
and pkg/reconciler/iploop_test.go
- each should have a different Describe
block.
This would address your current unit test error.
Pull Request Test Coverage Report for Build 2630891680Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
I'm ready to move forward on this change. Thanks for the hard work and all the follow up to get it in order. Well done.
Let's just squash the commits. Keep one commit for the vendor changes, and one more commit for the rest of the changes 👍
Signed-off-by: nicklesimba <[email protected]>
589c82f
to
545f055
Compare
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.
LGTM! 💯
This commit removes the IP Reconciler as a separate binary, and also removes the cronjob that launched it.
Instead, the code now invokes the reconciler code at a user configurable interval (once per day default).
As part of these changes, the code has also been reorganized with more clear file names etc.