[apiserversdk] check service belongs to kuberay #3563
Conversation
Signed-off-by: Troy Chiu <y.troychiu@gmail.com>
Signed-off-by: Troy Chiu <y.troychiu@gmail.com>
|
Hi @rueian can I get a review from you? Thank you. |
| return nil, err | ||
| } | ||
|
|
||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
curious about the decision here to choose anonymous function, instead of a normal function?
There was a problem hiding this comment.
I don't have a preference. Do you prefer a normal function?
Signed-off-by: Troy Chiu <y.troychiu@gmail.com>
Signed-off-by: Troy Chiu <y.troychiu@gmail.com>
apiserversdk/proxy.go
Outdated
| // This pattern allows accessing KubeRay dashboards and job submissions. | ||
| // Note: We register both "/proxy" and "/proxy/" to handle requests with and without trailing slashes. | ||
| // See https://pkg.go.dev/net/http#hdr-Trailing_slash_redirection-ServeMux |
There was a problem hiding this comment.
Can we move the comment to the test? We can have examples to show why avoiding trailing slash redirection matters there.
There was a problem hiding this comment.
do you mean keeping the comments in both place or entirely move to the test?
There was a problem hiding this comment.
I prefer to entirely move to the test.
There was a problem hiding this comment.
why moving to test? I thought this comment was telling people why we register two paths. I won't expect people read tests.
There was a problem hiding this comment.
Because we can have examples to show why avoiding trailing slash redirection matters there. The current statement "to handle requests with and without trailing slashes" alone doesn’t really show why avoiding the direction matters (it redirects non-GET requests to GET requests).
I don’t worry about people not reading the test. People who have doubts about the reason why we need two routes here will eventually find the test. However, if we really need a comment here, could we just say "to avoid the trailing slash redirection" instead of "to handle requests with and without trailing slashes" to avoid misunderstanding.
Signed-off-by: Troy Chiu <y.troychiu@gmail.com>
Signed-off-by: Troy Chiu <y.troychiu@gmail.com>
Why are these changes needed?
wrap the handler with a checker which make sure the service exists and belongs to kuberay.
End-to-end tests
service exists but doesn't belong to kuberay
service doesn't exist
service exists and belongs to kuberay
Related issue number
Closes #3521
Checks