[DoNotMerge] Ip reconciler alerting #218
Conversation
Signed-off-by: nicklesimba <[email protected]>
Signed-off-by: nicklesimba <[email protected]>
|
This pull request introduces 1 alert when merging a3a8599 into 611790c - view on LGTM.com new alerts:
|
|
First and foremost, if this PR has 2 objectives, I would rather have 2 PRs with one objective each. That will help us focus, and enable you to progress further faster. |
cmd/reconciler/ip.go
Outdated
| case <-time.After(5 * time.Second): | ||
| returnErr <- errors.New("timed out (this is the 5 sec test timeout, nbd)") | ||
| return |
There was a problem hiding this comment.
you don't need to do this. You should pass a context.WithTimeout below, in line https://github.com/k8snetworkplumbingwg/whereabouts/pull/218/files#diff-99080d2a71bc34fba51f6b2bb1766239380ae51bdf70aa1496fa73b5ffdbbe79R31
I also suspect that the issue you're facing comes from spawning 2 go routines
first in https://github.com/k8snetworkplumbingwg/whereabouts/pull/218/files#diff-dbddff16de1ff73a326e91a361beb31cae41142fa4b9e85d5e759575af2a56bdR98
second in https://github.com/k8snetworkplumbingwg/whereabouts/pull/218/files#diff-99080d2a71bc34fba51f6b2bb1766239380ae51bdf70aa1496fa73b5ffdbbe79R17
but I am not 100% sure of this second claim.
Signed-off-by: nicklesimba <[email protected]>
|
This pull request introduces 1 alert when merging a0f4d90 into 06d3340 - view on LGTM.com new alerts:
|
Signed-off-by: nicklesimba <[email protected]>
The 2 objectives here are actually pretty interlinked. I'm not ready to merge yet but I have both working at the simplest level, and I think separate PRs would end up becoming redundant since the code changes are small and are all made to work with each other. |
|
This pull request introduces 1 alert when merging 1052d80 into 2789b2e - view on LGTM.com new alerts:
|
…eory) ip_test.go should remain functional Signed-off-by: nicklesimba <[email protected]>
WIP pull request for ip-reconciler alerting. This PR has two goals: