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.

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

Resolve control plane endpoint when updating service #180

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

lubronzhan
Copy link
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes ##178

Describe testing done for PR:

Special notes for your reviewer:

Release note:


New PR Checklist

  • Ensure PR contains only public links or terms
  • Use good commit messages
  • Squash the commits in this branch before merge to preserve our git history
  • If this PR is just an idea or POC, use a Draft PR instead of a full PR
  • Add appropriate labels according to what type of issue is being addressed.

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 73.68421% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 36.54%. Comparing base (a70caef) to head (9be9129).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #180      +/-   ##
==========================================
+ Coverage   36.34%   36.54%   +0.19%     
==========================================
  Files          20       20              
  Lines        2724     2731       +7     
==========================================
+ Hits          990      998       +8     
  Misses       1675     1675              
+ Partials       59       58       -1     
Files Coverage Δ
pkg/ako-operator/lib.go 100.00% <ø> (ø)
pkg/haprovider/haprovider.go 68.96% <73.68%> (+0.93%) ⬆️

@lubronzhan lubronzhan force-pushed the topic/lubron/fix-178 branch from 2b6319b to c1a851a Compare March 13, 2024 22:24
Signed-off-by: Lubron Zhan <[email protected]>
Copy link
Contributor

@flawedmatrix flawedmatrix left a comment

Choose a reason for hiding this comment

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

For the most part, looks good. Just a question though, does the DNS resolver support IPv6?

@lubronzhan
Copy link
Contributor Author

lubronzhan commented Mar 14, 2024

For the most part, looks good. Just a question though, does the DNS resolver support IPv6?

yes it does

// LookupIP looks up host using the local resolver.
// It returns a slice of that host's IPv4 and IPv6 addresses.

Once we support ipv6 as control plane HA, when AKO supports it, we could add more tests

Copy link
Contributor

@flawedmatrix flawedmatrix left a comment

Choose a reason for hiding this comment

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

Yeah that makes sense. For some reason I thought this was part of the change


and I wasn't sure which IP family this returned.

But it seems like existing behavior so this PR looks good to me.

@lubronzhan lubronzhan merged commit 2f0649a into vmware-tanzu:main Mar 14, 2024
4 checks passed
@lubronzhan lubronzhan deleted the topic/lubron/fix-178 branch March 14, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants