Repository navigation
Conversation
NewTyped fills the value from the manager config first, so the `<= 0 -> 1` clamp in NewTypedUnmanaged is only reachable through NewUnmanaged. Nothing in the suite covers it: the existing specs assert the manager default, the unset default and an explicit value, all through controller.New. It is worth pinning because Start spawns exactly MaxConcurrentReconciles workers. At zero the controller starts, logs "Starting workers" with a count of 0 and reconciles nothing; at a negative count WaitGroup.Add panics. Either one turns a caller's typo into a controller that looks healthy. Verified by deleting the clamp: the test then fails on the -7 case, and passes again once it is restored.
|
|
Welcome @mgkvibez! |
kubernetes-prow
Bot
added
the
needs-ok-to-test
|
Hi @mgkvibez. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgkvibez The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
cncf-cla: no
|
/hold commented on the issue |
kubernetes-prow
Bot
added
the
do-not-merge/hold
Refs #3649.
Test-only, one file, no behaviour change. The clamp in
NewTypedUnmanagedis only reachable throughNewUnmanaged, and nothing covered it: the existing specs assert the manager default, the unset default and an explicit value, all throughcontroller.New.Worth pinning because
Startspawns exactlyMaxConcurrentReconcilesworkers. At zero the controller starts, logsStarting workerswith a count of 0 and reconciles nothing; at a negative countWaitGroup.Addpanics. Either way a caller's typo produces a controller that looks healthy.Verification the usual way: I removed the clamp and the test fails on the
-7case, restored it and the test passes.go test ./pkg/controller/ -run TestNewUnmanagedClampsMaxConcurrentReconciles,go vetandgofmtare clean. I did not run the full package suite locally because it needs envtest binaries, so the new test is standalone rather than joining the Ginkgo suite.One process note: CONTRIBUTING asks for an issue before a PR, hence #3649. Happy to fold this in there or rework it if you would rather the clamp returned an error than silently coerced.