-
Notifications
You must be signed in to change notification settings - Fork 345
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
Updates node configz and healthz endpoints #467
Conversation
pkg/discovery/nodes.go
Outdated
Resource("nodes"). | ||
Name(node.Name). | ||
SubResource("proxy"). | ||
Suffix("configz") |
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.
b/c the signatures are nearly identical, you could probably wrap into a generic proxy-getter closure or f(n).
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.
A reasonable request.
98b0f23
to
13018ba
Compare
@timothysc updated as per requested and refactored some logic I added TODOs with where I would like to see this go. My goal with these changes is to get something testable. If the entire kubernetes.Interface interface is a parameter of the function then you're out of luck for testing. I'd like to start pushing the big dependencies up to make the small pieces unit testable. |
pkg/discovery/nodes.go
Outdated
_, err = untypedQuery(out, "configz.json", func() (interface{}, error) { | ||
var configz map[string]interface{} | ||
_, err := untypedQuery(out, "configz.json", func() (interface{}, error) { | ||
result, err := getNodeEndpoint(restclient, name, "configz") |
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.
are you ignoring the error here?
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.
b/c it could be overridden | masked by the raw call below.
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.
yes, the error is ignored, thanks for 👀
/hold |
/hold cancel This is good to go. It's two commits as opposed to two PRs since the second commit depends on the first but is a logically separate issue. |
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.
minor nit otherwise lgtm.
merge at will.
pkg/discovery/nodes.go
Outdated
SubResource("proxy"). | ||
Suffix(endpoint) | ||
|
||
// Get the configz endpoint, put the result in the nodeData |
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 comment above no longer makes sense, since it's generic.
Fixes vmware-tanzu#465 The proxy subresource was moved in k8s v1.10 Signed-off-by: Chuck Ha <[email protected]>
Fixes vmware-tanzu#475 Signed-off-by: Chuck Ha <[email protected]>
Fixes #465
The proxy subresource was moved in k8s v1.10
Signed-off-by: Chuck Ha [email protected]
What this PR does / why we need it:
This fixes the configz and healthz outputs. They were null as of k8s 1.10. However, no one really noticed so I wonder how many people use or need this feature.
Special notes for your reviewer:
Release note: