-
Notifications
You must be signed in to change notification settings - Fork 40.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
Remove unused (?) HealthCheck from KubeletClient #12913
Conversation
GCE e2e build/test passed for commit 33c894a. |
@@ -52,6 +52,7 @@ func proxyContext(version string) { | |||
f := NewFramework("proxy") | |||
prefix := "/api/" + version | |||
|
|||
// Port here has to be kept in sync with default kubelet port. |
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.
you could always format it with the const
https://github.com/kubernetes/kubernetes/blob/master/pkg/master/ports/ports.go#L28
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 don't understand what you mean.
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.
Kind of a nit/suggestion. What I mean is the next line could be:
It("should proxy logs on node with explicit kubelet port", func() {
nodeProxyTest(f, version, fmt.Sprintf(":%v/logs/", ports.KubeletPort))
})
Then no need for the comment and we only have one thing to keep in sync which is the pkg/master/ports constants.
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.
This will be no longer true and each Kubelet will be able to have different port.
LGTM. Thanks for the cleanup! |
Remove unused (?) HealthCheck from KubeletClient
Needed for #12912
cc @dchen1107 @yujuhong @wojtek-t