Skip to content

Conversation

@MortalHappiness
Copy link
Member

@MortalHappiness MortalHappiness commented Jan 6, 2025

Why are these changes needed?

See the description in the corresponding issue for details.

Related issue number

Closes: #2706

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@MortalHappiness MortalHappiness force-pushed the feature/#2706-refactor-clusteraction branch 2 times, most recently from f109493 to 7bb1438 Compare January 6, 2025 18:03
@MortalHappiness MortalHappiness marked this pull request as ready for review January 6, 2025 18:03
Closes: ray-project#2706
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness MortalHappiness force-pushed the feature/#2706-refactor-clusteraction branch from 7bb1438 to 579310c Compare January 6, 2025 18:05
@kevin85421
Copy link
Member

cc @rueian would you mind reviewing this PR?

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the PR! Very clean and well tested!

Funny GIF

return rayServiceInstance, nil
}

func isZeroDowntimeUpgradeEnabled(ctx context.Context, rayService *rayv1.RayService) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add unit tests for this function.

Copy link
Member Author

@MortalHappiness MortalHappiness Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already covered in this test

func TestReconcileRayCluster(t *testing.T) {

@rueian
Copy link
Collaborator

rueian commented Jan 8, 2025

LGTM.

Closes: ray-project#2706
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@kevin85421 kevin85421 merged commit 7bb5877 into ray-project:master Jan 8, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RayService][refactor] Unify the logic of shouldPrepareNewRayCluster and createRayClusterInstanceIfNeeded

3 participants