-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Get controller.go test coverage to 100% #9
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.
LGTM, minor comments. Feel free to self-merge it after fixing those comments.
pkg/controller/controller.go
Outdated
@@ -93,6 +93,8 @@ type controller struct { | |||
// the queue for processing | |||
queue workqueue.RateLimitingInterface | |||
|
|||
jitterPeriod time.Duration |
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.
May be a comment explaining what is this really for ?
pkg/controller/controller.go
Outdated
@@ -145,21 +149,33 @@ func (c *controller) Start(stop <-chan struct{}) error { | |||
for _, informer := range allInformers { | |||
syncedFuncs = append(syncedFuncs, informer.HasSynced) | |||
} | |||
if ok := cache.WaitForCacheSync(stop, syncedFuncs...); !ok { | |||
|
|||
// Allow waitForCache to be mocked out for tests |
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.
May be move this comment where this is declared ?
pkg/controller/controller.go
Outdated
go wait.Until(func() { | ||
// TODO(pwittrock): Should we really use wait.Until to continuously restart this if it exits? | ||
fmt.Printf("Running\n") |
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.
fmt --> log ?
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.
These were debug statements :P
pkg/controller/controller.go
Outdated
for c.processNextWorkItem() { | ||
} | ||
}, time.Second, stop) | ||
fmt.Printf("Stopping\n") |
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.
fmt --> log ?
|
||
}) | ||
}) | ||
}) |
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.
nit: filename has "manger" --> manager :)
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.
Should Describe
be "controller manager", and //TODO
comments be needed ?
/lgtm |
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
|
||
}) | ||
}) | ||
}) |
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.
Should Describe
be "controller manager", and //TODO
comments be needed ?
New changes are detected. LGTM label has been removed. |
Get controller.go test coverage to 100%
Better help messaging for kubebuilder
No description provided.