-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(translator): implement timeout in ClientTrafficPolicy #2667
Conversation
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2667 +/- ##
==========================================
+ Coverage 63.38% 63.41% +0.02%
==========================================
Files 119 119
Lines 19098 19156 +58
==========================================
+ Hits 12106 12147 +41
- Misses 6193 6212 +19
+ Partials 799 797 -2 ☔ View full report in Codecov by Sentry. |
return err | ||
} | ||
|
||
if httpIR.Timeout == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, but maybe this is slightly more readable as a switch statement:
switch {
case httpIR.Timeout == nil:
httpIR.Timeout = &ir.ClientTimeout{}
fallthrough
case httpIR.Timeout.HTTP == nil:
httpIR.Timeout.HTTP = &ir.HTTPClientTimeout{}
}
httpIR.Timeout.HTTP.RequestReceivedTimeout = &metav1.Duration{
Duration: d,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Lior
I think that this suggestion offers a cleaner structure, but is it a convention for implementing switch cases? I'm not sure that all readers are familiar with the fallthrough
keyword.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallthrough
keyword is part of the language - readers should be familiar with the language.
But this can also be written without it:
switch {
case httpIR.Timeout == nil:
httpIR.Timeout = &ir.ClientTimeout{
HTTP: &ir.HTTPClientTimeout{},
}
case httpIR.Timeout.HTTP == nil:
httpIR.Timeout.HTTP = &ir.HTTPClientTimeout{}
}
httpIR.Timeout.HTTP.RequestReceivedTimeout = &metav1.Duration{
Duration: d,
}
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, looking for doc and e2e with following PRs.
What type of PR is this?
Implement Timeout in ClientTrafficPolicy
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #2598