[refactor] Refactor enable login shell #3704
Conversation
Signed-off-by: kaihsun <[email protected]>
| @@ -202,13 +201,6 @@ func TestGetSubmitterTemplate(t *testing.T) { | |||
| envVar, found = utils.EnvVarByName(utils.RAY_JOB_SUBMISSION_ID, submitterTemplate.Spec.Containers[utils.RayContainerIndex].Env) | |||
| assert.True(t, found) | |||
| assert.Equal(t, "test-job-id", envVar.Value) | |||
|
|
|||
There was a problem hiding this comment.
TestGetContainerCommand already tests the flag, and this test covers the behavior of Command. Therefore, "Test 7" can be removed.
| @@ -3007,111 +3008,6 @@ func Test_RedisCleanup(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func Test_RedisCleanupWithLoginShell(t *testing.T) { | |||
There was a problem hiding this comment.
TestGetContainerCommand already tests the flag. It is not worth it to maintain this complex test, so I move the check to Test_RedisCleanup.
We should refactor the interface to make write unit tests easier.
| if initContainer.Name == "wait-gcs-ready" { | ||
| assert.Equal(t, []string{"/bin/bash", "-lc", "--"}, initContainer.Command) | ||
| } | ||
| func TestBuildPod_WithLoginBash(t *testing.T) { |
There was a problem hiding this comment.
There is no test to verify container command in pod_test.go, so I didn't delete this test.
| @@ -554,9 +554,9 @@ func getSubmitterTemplate(ctx context.Context, rayJobInstance *rayv1.RayJob, ray | |||
| if err != nil { | |||
| return corev1.PodTemplateSpec{}, err | |||
| } | |||
| submitterTemplate.Spec.Containers[utils.RayContainerIndex].Command = utils.GetContainerCommand()[:2] | |||
There was a problem hiding this comment.
It's weird to process utils.GetContainerCommand(). We should unify all command generation related logic into the util function.
| @@ -703,9 +703,12 @@ func GetClusterType() bool { | |||
| return false | |||
| } | |||
|
|
|||
| func GetContainerCommand() []string { | |||
| func GetContainerCommand(additionalOptions []string) []string { | |||
There was a problem hiding this comment.
nit: Add a comment that additionalOptions only accepts flags with single character.
|
cc @fscnick |
Signed-off-by: kaihsun <[email protected]>
Why are these changes needed?
A follow up of #3679
Related issue number
Checks