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

feat: add k8s topology zone info for nodePort service #4301

Closed
wants to merge 1 commit into from
Closed

feat: add k8s topology zone info for nodePort service #4301

wants to merge 1 commit into from

Conversation

kolorful
Copy link
Contributor

@kolorful kolorful commented Sep 3, 2024 •
edited
Loading

Changes proposed in this PR

How I've tested this PR

  • Unit test
  • Running locally compiled version on a production Kubernetes cluster and verified it worked (see attached screenshot)
image

How I expect reviewers to test this PR

  • Same as how I tested

Checklist

@kolorful kolorful force-pushed the main branch 2 times, most recently from 04ebc1e to dc24ff2 Compare September 3, 2024 16:39
@kolorful kolorful marked this pull request as ready for review September 3, 2024 17:06
@kolorful kolorful requested a review from a team as a code owner November 18, 2024 14:52
@xwa153
Copy link
Member

xwa153 commented Jan 31, 2025

Hi @kolorful thank you for your nice catch! I tested and the code looks perfect!

I added one small change in the test, but I would suggest we add more tests similarly to what you have done in your first PR. I recommend we add more tests in following functions to cover more scenarios:

  • TestServiceResource_lbRegisterEndpoints
  • TestServiceResource_clusterIPPrefix
  • TestServiceResource_clusterIPAnnotatedPortName
  • TestServiceResource_clusterIPAnnotatedPortNumber
  • TestServiceResource_clusterIPUnnamedPorts
  • TestServiceResource_clusterIPAllNamespaces
  • TestServiceResource_clusterIPTargetPortNamed

Make sure you pull the latest main branch before getting started. Thank you again for your contribution!

@kolorful
Copy link
Contributor Author

@xwa153 thanks for reviewing the code! I've rebased upstream, maded the requested updates. Would you mind taking a another look?

@xwa153
Copy link
Member

xwa153 commented Jan 31, 2025 •
edited
Loading

@kolorful Thank you for your hard work! All look great! 👍

@xwa153
Copy link
Member

xwa153 commented Jan 31, 2025

Hi @kolorful

We need to pass the CI before merging into main. But the CI needs the internal credentials. I cherry-picked your commit and opened another PR in order to pass the CI. The changes are identical except for the file name of the changelog to reflect the number of the PR. We will close this PR after that one is merged. Please let me know if you have any question.

Thanks for your contribution.

@xwa153 xwa153 mentioned this pull request Jan 31, 2025
Merged
xwa153 added a commit that referenced this pull request Feb 3, 2025
@xwa153 @kolorful
feat: add k8s topology zone info for nodePort service(author @kolorful)…
b7f2b70 
… (#4470)

* feat: add k8s topology zone info for nodePort service

* original PR: #4301

---------

Co-authored-by: kolorful <[email protected]>
@xwa153
Copy link
Member

xwa153 commented Feb 3, 2025

PR merged

@xwa153 xwa153 closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xwa153 xwa153 xwa153 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kolorful @xwa153