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

Tweaked order of ingress IPs in ServiceLB #8711

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

jsnctl
Copy link
Contributor

@jsnctl jsnctl commented Oct 22, 2023

Previously, ingress IPs were only string-sorted when returned

Sorted by IP family and string-sorted in each family as part of filterByIPFamily method

Proposed Changes

Addresses #8704 with adjustment and use of filterByIPFamily as part of servicelb.getStatus

Types of Changes

Bugfix

Verification

Added new unit test to servicelb_test.go

Testing

See above

Linked Issues

User-Facing Change

Improved ingress IP ordering from ServiceLB

Further Comments

Hopefully my first PR of many. Great project

Previously, ingress IPs were only string-sorted when returned

Sorted by IP family and string-sorted in each family as part of
filterByIPFamily method

Signed-off-by: Jason Costello <jason@jsnc.tl>
Copy link
Contributor

@brandond brandond left a comment

Choose a reason for hiding this comment

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

thanks! one nit but lgtm otherwise.

pkg/cloudprovider/servicelb.go Outdated Show resolved Hide resolved
Co-authored-by: Brad Davidson <brad@oatmail.org>
Signed-off-by: Jason Costello <jason@hazy.com>
@jsnctl
Copy link
Contributor Author

jsnctl commented Oct 24, 2023

CI is failing but see that in other PRs so guess this is a known issue? Holding off merge until code freeze is up too?

@brandond
Copy link
Contributor

brandond commented Oct 24, 2023

No, it looks like your formatting is off. Editors will usually handle this for you, if yours does not you may need to use the gofmt CLI tool to fix it.

pkg/cloudprovider/servicelb.go:288: File is not `gofmt`-ed with `-s` (gofmt)`

Signed-off-by: Jason Costello <jason@jsnc.tl>
@jsnctl jsnctl force-pushed the servicelb-ingress-ips-better-sorting branch from 01a08ea to fccc6f3 Compare October 25, 2023 19:28
@jsnctl
Copy link
Contributor Author

jsnctl commented Nov 2, 2023

Anything further to pick up on this PR with the formatting fixed @brandond?

@brandond
Copy link
Contributor

brandond commented Nov 2, 2023

no, we're just waiting on release activity before merging any PRs. Thanks for your patience!

@brandond
Copy link
Contributor

s390x is flaking; merging

@brandond brandond merged commit 07ee854 into k3s-io:master Nov 15, 2023
13 of 14 checks passed
brandond added a commit to brandond/k3s that referenced this pull request Nov 16, 2023
* Tweaked order of ingress IPs in ServiceLB
    Previously, ingress IPs were only string-sorted when returned
    Sorted by IP family and string-sorted in each family as part of
    filterByIPFamily method
* Update pkg/cloudprovider/servicelb.go
* Formatting

Signed-off-by: Jason Costello <jason@hazy.com>
Co-authored-by: Brad Davidson <brad@oatmail.org>
(cherry picked from commit 07ee854)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to brandond/k3s that referenced this pull request Nov 16, 2023
* Tweaked order of ingress IPs in ServiceLB
    Previously, ingress IPs were only string-sorted when returned
    Sorted by IP family and string-sorted in each family as part of
    filterByIPFamily method
* Update pkg/cloudprovider/servicelb.go
* Formatting

Signed-off-by: Jason Costello <jason@hazy.com>
Co-authored-by: Brad Davidson <brad@oatmail.org>
(cherry picked from commit 07ee854)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to brandond/k3s that referenced this pull request Nov 16, 2023
* Tweaked order of ingress IPs in ServiceLB
    Previously, ingress IPs were only string-sorted when returned
    Sorted by IP family and string-sorted in each family as part of
    filterByIPFamily method
* Update pkg/cloudprovider/servicelb.go
* Formatting

Signed-off-by: Jason Costello <jason@hazy.com>
Co-authored-by: Brad Davidson <brad@oatmail.org>
(cherry picked from commit 07ee854)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit that referenced this pull request Nov 16, 2023
* Tweaked order of ingress IPs in ServiceLB
    Previously, ingress IPs were only string-sorted when returned
    Sorted by IP family and string-sorted in each family as part of
    filterByIPFamily method
* Update pkg/cloudprovider/servicelb.go
* Formatting

Signed-off-by: Jason Costello <jason@hazy.com>
Co-authored-by: Brad Davidson <brad@oatmail.org>
(cherry picked from commit 07ee854)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit that referenced this pull request Nov 16, 2023
* Tweaked order of ingress IPs in ServiceLB
    Previously, ingress IPs were only string-sorted when returned
    Sorted by IP family and string-sorted in each family as part of
    filterByIPFamily method
* Update pkg/cloudprovider/servicelb.go
* Formatting

Signed-off-by: Jason Costello <jason@hazy.com>
Co-authored-by: Brad Davidson <brad@oatmail.org>
(cherry picked from commit 07ee854)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit that referenced this pull request Nov 16, 2023
* Tweaked order of ingress IPs in ServiceLB
    Previously, ingress IPs were only string-sorted when returned
    Sorted by IP family and string-sorted in each family as part of
    filterByIPFamily method
* Update pkg/cloudprovider/servicelb.go
* Formatting

Signed-off-by: Jason Costello <jason@hazy.com>
Co-authored-by: Brad Davidson <brad@oatmail.org>
(cherry picked from commit 07ee854)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
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.

3 participants