fix crash when pingsource adapter is scaled up without any sources#4242
Conversation
knative-prow-robot
added
size/XS
| value: '' | ||
| - name: K_SINK_TIMEOUT | ||
| value: '' | ||
| value: '0' |
There was a problem hiding this comment.
Wouldn't it be more graceful to handle an empty K_SINK_TIMEOUT instead? E.g. default to 0 if K_SINK_TIMEOUT == ""
There was a problem hiding this comment.
We could but I don't think that's necessary. This is not the only env var that will cause a pod to crash is not set properly by the operator.
There was a problem hiding this comment.
I'm not too comfortable with that, and it's exactly what is called the broken window theory: if we have something that's already shaky there is less remorse to introduce another shaky piece to the puzzle.
Just because it's 0 in config/ doesn't mean every user will use that manifest. Sooner or later someone will omit the var and report that same crash again.
We can make things right about K_SINK_TIMEOUT in particular in this PR, and tackle other occurrences of unsafe env var handling in subsequent PRs. I'm happy to help if it's just a matter of time for you.
There was a problem hiding this comment.
yup agree. Fixing...
Codecov Report
@@ Coverage Diff @@
## master #4242 +/- ##
=======================================
Coverage 79.93% 79.93%
=======================================
Files 277 277
Lines 7725 7725
=======================================
Hits 6175 6175
Misses 1170 1170
Partials 380 380 Continue to review full report at Codecov.
|
|
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-eventing-upgrade-tests: |
|
You need add an e2e test for this. |
|
agree for the e2e test. However it's a big effort for an edge case and a bit risky for a fix that needs to be backported. |
knative-prow-robot
added
size/M
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: antoineco, lionelvillard The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
…native#4242) * fix crash when pingsource adapter is scaled up without any sources * K_SINK_TIMEOUT now accepts any string value
…native#4242) (#1020) * fix crash when pingsource adapter is scaled up without any sources * K_SINK_TIMEOUT now accepts any string value Co-authored-by: Lionel Villard <villard@us.ibm.com>
6 participants
No description provided.