Log a warning when remote-deployments-poll-interval gets clamped - #2354
magic-peach wants to merge 1 commit into
Conversation
The flag help text and README both document that values below 1s fall back to the default with a warning, but the code only clamped the value silently, so a misconfigured flag gave no signal at all. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: magic-peach 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 |
|
Welcome @magic-peach! It looks like this is your first PR to knative/operator 🎉 |
knative-prow
Bot
added
size/M
|
Hi @magic-peach. Thanks for your PR. I'm waiting for a knative 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. |
|
/ok-to-test |
knative-prow
Bot
added
ok-to-test
|
/lgtm |
| // RemoteDeploymentsPollIntervalValue returns the configured poll interval, clamping values below 1s to the default. | ||
| func RemoteDeploymentsPollIntervalValue() time.Duration { | ||
| if remoteDeploymentsPollIntervalFlag < time.Second { | ||
| return defaultRemoteDeploymentsPollInterval | ||
| } | ||
| return remoteDeploymentsPollIntervalFlag | ||
| } |
There was a problem hiding this comment.
I'd rather see bool returned from this function, as it states clamping is happening. Then act accordingly to log warning.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2354 +/- ##
==========================================
- Coverage 65.95% 65.88% -0.08%
==========================================
Files 56 56
Lines 2682 2688 +6
==========================================
+ Hits 1769 1771 +2
- Misses 777 781 +4
Partials 136 136 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
This repo has no PR template, so this is a plain factual description.
What changed: added RemoteDeploymentsPollIntervalWasClamped in pkg/reconciler/common/poll_interval_flag.go, and a conditional logger.Warnf call in both pkg/reconciler/knativeserving/controller.go and pkg/reconciler/knativeeventing/controller.go, right next to the existing startup log line that reports the resolved poll interval.
Why: the flag help text and the README's CLI flags table both document that a remote-deployments-poll-interval value below 1s falls back to the default with a warning log entry. The code only clamped the value silently, so a misconfigured flag gave no signal at all. Confirmed via git log that this was an oversight in the PR that introduced the flag (#2267), not something already tracked.
How tested: go build ./... and go test on pkg/reconciler/common, pkg/reconciler/knativeserving, and pkg/reconciler/knativeeventing, all green, including 9 new and existing cases for the poll interval helpers. Rebuilt and retested against the current tip of main immediately before opening this PR.
Confirmed no existing issue or PR covers this (searched by flag name and by keyword).