[Refactor] Extract KubectlApplyYaml and yaml deserialization to support package #2498
Conversation
…rt package Closes: ray-project#2497 Signed-off-by: Chi-Sheng Liu <[email protected]>
MortalHappiness
force-pushed
the
feature/#2497-refactor-yaml-support-func
branch
from
6d12af1
to
4ecff73
Compare
| @@ -85,7 +87,6 @@ func TestRayCluster(t *testing.T) { | |||
| } | |||
| } | |||
| g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(HaveLen(int(desiredWorkerReplicas))) | |||
| g.Expect(rayCluster.Status.DesiredWorkerReplicas).To(Equal(desiredWorkerReplicas)) | |||
| g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(HaveLen(int(desiredWorkerReplicas))) | ||
| g.Expect(rayCluster.Status.DesiredWorkerReplicas).To(Equal(desiredWorkerReplicas)) |
There was a problem hiding this comment.
I think we don't need to check this because we've already checked the number of worker pods above. Also because this is a one-line fix so I do it directly in this refactoring PR. Do you think it's better to create a separate PR?
There was a problem hiding this comment.
If I understand correctly, this differs from the above logic. The above logic checks the number of worker Pods by querying the K8s API server, which is the source of truth. The deleted line checks whether the RayCluster status reflects the actual cluster status.
There was a problem hiding this comment.
Okay I've restored this check, but updating it to
g.Expect(GetRayCluster(test, namespace.Name, rayCluster.Name)).To(WithTransform(RayClusterDesiredWorkerReplicas, Equal(desiredWorkerReplicas)))because I found that we forgot to get the RayCluster again to update its status.
There was a problem hiding this comment.
Using a transform just to return a field seems overkill :P
There was a problem hiding this comment.
I used WithTransform because I found there already been a RayClusterDesiredWorkerReplicas function.
There was a problem hiding this comment.
And GetRayCluster returns 2 parameters: one is raycluster, and the other is error. WithTransform can successfully take only the first one as input and fail if err is not nil. I think this is an ideal and neat syntax.
Closes: ray-project#2497 Signed-off-by: Chi-Sheng Liu <[email protected]>
| g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(HaveLen(int(desiredWorkerReplicas))) | ||
| g.Expect(rayCluster.Status.DesiredWorkerReplicas).To(Equal(desiredWorkerReplicas)) |
There was a problem hiding this comment.
Using a transform just to return a field seems overkill :P
Why are these changes needed?
See the description in the corresponding issue for details.
Related issue number
Closes: #2497
Checks