Fix configuration timeout defaulting #15617
Conversation
knative-prow
bot
added
the
size/L
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15617 +/- ##
==========================================
- Coverage 80.80% 80.79% -0.01%
==========================================
Files 222 222
Lines 18034 18044 +10
==========================================
+ Hits 14572 14579 +7
- Misses 3090 3093 +3
Partials 372 372 ☔ View full report in Codecov by Sentry. |
|
@dprotaso gentle ping. |
| Name: config.DefaultsConfigName, | ||
| }, | ||
| Data: map[string]string{ | ||
| "revision-timeout-seconds": "423", |
There was a problem hiding this comment.
Why was 423 chosen as the timeout value? Consider using a named constant or variable to make the
intent clear and maintain consistency across test files.
There was a problem hiding this comment.
Just picked a number. I will update with a constant.
| }, | ||
| }, | ||
| }, | ||
| ctx: func() context.Context { |
There was a problem hiding this comment.
The context setup logic is duplicated between configuration_defaults_test.go and
revision_defaults_test.go. Consider extracting this into a test helper function to improve
maintainability.
There was a problem hiding this comment.
Actually they are using a different config store although the func seems similar, which makes things harder because configs are of different types and don't inherit similar methods. I will give it a shot.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: skonto The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
skonto
force-pushed
the
response_time_fix
branch
from
610bdc8 to
308d56c
Compare
|
cc @dprotaso |
|
/retest |
Fixes #15616
Proposed Changes
revision-response-start-timeout-seconds,revision-idle-timeout-seconds, so that when they are equal to therevision-timeout-secondsin the defaults cm, they will not be set to 300 (default) at a revision that has no overrides of those values. Instead they are set to nil at the revision (0at the QP side). Semantically this is correct, since when the timeouts are equal we don't need both anyway.Release Note