-
Notifications
You must be signed in to change notification settings - Fork 369
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 http health check filter #3596
Conversation
Signed-off-by: Dingkang Li <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3596 +/- ##
==========================================
+ Coverage 68.22% 68.33% +0.10%
==========================================
Files 168 169 +1
Lines 20560 20618 +58
==========================================
+ Hits 14028 14090 +62
+ Misses 5520 5516 -4
Partials 1012 1012 ☔ View full report in Codecov by Sentry. |
thanks @aoledk this is great, can you also add an e2e for this that pings the healthCheck endpoint and gets a |
Signed-off-by: Dingkang Li <[email protected]>
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! Just one comment on the naming.
test/e2e/tests/health_check.go
Outdated
ConformanceTests = append(ConformanceTests, HealthCheckTest) | ||
} | ||
|
||
var HealthCheckTest = suite.ConformanceTest{ |
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.
can we rename this into DownstreamHealthCheck
? Since this health check is in CTP, as we also have health check in BTP, and the UpstreamHealthCheck
e2e test case will be added soon.
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.
+1 prefer ListenerHealthCheck
Signed-off-by: Dingkang Li <[email protected]>
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 thanks !
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! the linter need a fix :)
Signed-off-by: Dingkang Li <[email protected]>
What this PR does / why we need it:
Follow #3540
Which issue(s) this PR fixes:
Fixes #3258