[refactor][3/N] Refactor dashbpard httpclient #3992
Conversation
Signed-off-by: You-Cheng Lin (Owen) <[email protected]>
Signed-off-by: You-Cheng Lin (Owen) <[email protected]>
Signed-off-by: You-Cheng Lin (Owen) <[email protected]>
owenowenisme
changed the title
owenowenisme
requested review from
kevin85421,
andrewsykim,
rueian and
MortalHappiness
as code owners
| return runtimeEnv, nil | ||
| } | ||
|
|
||
| func GetRayDashboardClientFunc(mgr manager.Manager, useKubernetesProxy bool) func(rayCluster *rayv1.RayCluster, url string) RayDashboardClientInterface { |
There was a problem hiding this comment.
This function is AI-generated. Please check whether this is correct or not. For example, the logic to get head service name is different from the old one.
There was a problem hiding this comment.
Really sorry about this, I'll make sure it won't happen again in the following PRs.
| } | ||
|
|
||
| func GetRayDashboardClientFunc(mgr manager.Manager, useKubernetesProxy bool) func(rayCluster *rayv1.RayCluster, url string) RayDashboardClientInterface { | ||
| return func(rayCluster *rayv1.RayCluster, url string) RayDashboardClientInterface { |
There was a problem hiding this comment.
The interface could be improved, but since it’s the same as InitClient, it doesn’t add new tech debt.
| return func(rayCluster *rayv1.RayCluster, url string) RayDashboardClientInterface { | ||
| if useKubernetesProxy { | ||
| return &RayDashboardClient{ | ||
| client: mgr.GetHTTPClient(), |
There was a problem hiding this comment.
what's the difference between mgr.GetHTTPClient and http.Client{ ... }?
There was a problem hiding this comment.
Since we are using Kubernetes proxy, we should use mgr.GetHTTPClient which provides client with proper auth to make request to Kubernetes api server.
Or we will get error like this:
{"level":"error","ts":"2025-08-28T03:13:20.716Z","logger":"controllers.RayJob",
"msg":"Failed to get job info",
"RayJob":{"name":"rayjob-sample","namespace":"default"},"reconcileID":"6b31ed8f-4d3c-4d39-93b2-3e8a2741ce43","JobId":"rayjob-sample-zhmhv",
"error":"Get \"https://127.0.0.1:38963/api/v1/namespaces/default/services/rayjob-sample-dksrt-head-svc:dashboard/proxy/api/jobs/rayjob-sample-zhmhv\": tls: failed to verify certificate: x509: certificate signed by unknown authority"}
| return headServiceURL, nil | ||
| } | ||
|
|
||
| func UnmarshalRuntimeEnvYAML(runtimeEnvYAML string) (RuntimeEnvType, error) { |
There was a problem hiding this comment.
Please avoid mixing too many different topics in the same PR. This makes reviewers more difficult to review. Please remove the change.
| return r.SubmitJobReq(ctx, request) | ||
| } | ||
|
|
||
| func (r *RayDashboardClient) SubmitJobReq(ctx context.Context, request *RayJobRequest, name *string) (jobId string, err error) { |
There was a problem hiding this comment.
Can you open another PR to remove the logs instead?
There was a problem hiding this comment.
Sure, will do when this is merged.
Signed-off-by: You-Cheng Lin (Owen) <[email protected]>
owenowenisme
force-pushed
the
refactor-dashbpard-httpclient
branch
from
e80d6a5 to
558dc64
Compare
Signed-off-by: You-Cheng Lin (Owen) <[email protected]>
Why are these changes needed?
Remove the controller-runtime from
dashboard-httpclientand remove the redundantInitClientfunction.Related issue number
Checks