-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Adds definitions to backend kv template for health checking #1644
Conversation
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.
Great!
Minor improvement possible in the template ;)
Other than that, looks good!
templates/kv.tmpl
Outdated
@@ -19,6 +19,14 @@ | |||
sticky = {{$sticky}} | |||
{{end}} | |||
|
|||
{{$healthCheck := Get "" . "/healthcheck/" "path"}} | |||
{{$interval := Get "30s" . "/healthcheck/" "interval"}} | |||
{{with $healthCheck}} |
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.
We may move {{$interval := Get "30s" . "/healthcheck/" "interval"}}
below {{with $healthCheck}}
?
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.
I followed the same format as the load balancer lines above (where {{$sticky := Get "false" . "/loadbalancer/" "sticky"}}
is outside the with
). I can move it inside the {{with $healthCheck}}
if you want.
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.
It's not mandatory, but it would be better :) And yes, we could do the same thing with sticky.
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.
@emilevauge I moved the interval
Get
into the with
block.
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!
LGTM
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 @zachomedia
LGTM
dff989c
to
5917e1a
Compare
Description
This PR adds definitions to the KV template to load backend health check information (path and interval).
I've only tested this change with the etcd backend.