Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix controller: use Service's TargetPort #383

Merged
merged 1 commit into from
Jul 25, 2022
Merged

fix controller: use Service's TargetPort #383

merged 1 commit into from
Jul 25, 2022

Conversation

davidxia
Copy link
Contributor

@davidxia davidxia commented Jul 15, 2022 •
edited
Loading

if NodePort is "0" for updating RayCluster's status.endpoints.

follow up to #341

Checks

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

@davidxia davidxia mentioned this pull request Jul 16, 2022
Merged
4 tasks
@davidxia davidxia force-pushed the dxia/patch1 branch 2 times, most recently from 5e36b07 to 0e99561 Compare July 16, 2022 12:59
@davidxia davidxia marked this pull request as ready for review July 16, 2022 13:46
@davidxia
Copy link
Contributor Author

Do I need to update ray-operator/controllers/ray/raycluster_controller_test.go. This is an integration test that creates real RayCluster, right? Any way to run locally? Didn't see in docs at first glance.

@Jeffwan
Copy link
Collaborator

Jeffwan commented Jul 19, 2022

@davidxia

Do I need to update ray-operator/controllers/ray/raycluster_controller_test.go. This is an integration test that creates real RayCluster, right? Any way to run locally? Didn't see in docs at first glance.

I normally build images and test locally in docker for desktop. Do you expect that guidance or something similiar to make test command?

@davidxia
Copy link
Contributor Author

davidxia commented Jul 20, 2022 •
edited
Loading

Do you expect that guidance or something similiar to make test command?

@Jeffwan Yea even a brief mention in dev docs will be helpful.

@davidxia davidxia force-pushed the dxia/patch1 branch 3 times, most recently from ff2a978 to fa79d31 Compare July 21, 2022 19:58
@davidxia
fix controller: use Service's TargetPort
c68e7b8 
if NodePort is "0" for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
scarlet25151
scarlet25151 approved these changes Jul 21, 2022
View reviewed changes
Copy link
Collaborator

@scarlet25151 scarlet25151 left a comment

Choose a reason for hiding this comment

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

Overall it's LTGM now. /cc @Jeffwan

@davidxia davidxia mentioned this pull request Jul 24, 2022
Closed
@Jeffwan Jeffwan added this to the v0.3.0 release milestone Jul 25, 2022
@Jeffwan
Copy link
Collaborator

Jeffwan commented Jul 25, 2022

I notice this problem in our downstream as well. Thanks for the fix. @davidxia

@Jeffwan Jeffwan merged commit 8dcc8a7 into ray-project:master Jul 25, 2022
@davidxia davidxia deleted the dxia/patch1 branch August 12, 2022 21:58
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
@davidxia
fix controller: use Service's TargetPort (ray-project#383)
7492e85 
if NodePort is "0" for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scarlet25151 scarlet25151 scarlet25151 approved these changes

@Jeffwan Jeffwan Jeffwan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.3.0 release
Development

Successfully merging this pull request may close these issues.
3 participants
@davidxia @Jeffwan @scarlet25151