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

Add support for EndpointSlices #3260

Merged
merged 39 commits into from
Nov 24, 2022
Merged

Add support for EndpointSlices #3260

merged 39 commits into from
Nov 24, 2022

Conversation

shaun-nx
Copy link
Contributor

@shaun-nx shaun-nx commented Nov 16, 2022

Proposed changes

This PR updates the Ingress Controller to support EndpointSlices

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@shaun-nx shaun-nx changed the title Endpoint slices Add support for EndpointSlices Nov 16, 2022
@shaun-nx shaun-nx marked this pull request as ready for review November 16, 2022 14:08
@shaun-nx shaun-nx requested a review from a team as a code owner November 16, 2022 14:08
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022

Codecov Report

Merging #3260 (704dcba) into main (4464caf) will increase coverage by 0.22%.
The diff coverage is 45.28%.

@@            Coverage Diff             @@
##             main    #3260      +/-   ##
==========================================
+ Coverage   52.66%   52.89%   +0.22%     
==========================================
  Files          59       59              
  Lines       16117    16137      +20     
==========================================
+ Hits         8488     8535      +47     
+ Misses       7345     7316      -29     
- Partials      284      286       +2     
Impacted Files Coverage Δ
internal/k8s/handlers.go 6.83% <0.00%> (ø)
internal/k8s/task_queue.go 0.00% <0.00%> (ø)
internal/k8s/utils.go 14.08% <0.00%> (-0.63%) ⬇️
internal/k8s/controller.go 12.85% <56.47%> (+1.57%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shaun-nx shaun-nx requested review from ciarams87 and jjngx November 22, 2022 11:39
Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

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

how about:

func TestGetEndpointsFromEndpointSlieces_ErrorsOnInvalidInput(t *testing.T) {
// content goes here
}

Since we are interested to test if the method errors on invalid inputs (sourced from "table tests") how about:

_, err := lbc.getEndpointsForPortFromEndpointSlices(....)
if err == nil {
    t.Error("want error, got nil")
}

Since we are interested in the fact that the func returns an error, lines 743...745 seem to be of no use.

In general, it may be worth to leverage cmp.Equal method since we use the cmp package anyway. How about:

if !cmp.Equal(want, got) {
    t.Error(cmp.Diff(want, got))
}

Note, that we defer calculating Diff, and do so only if the test fails. Checking for equality is cheeper comparing to calculating Diff.

Just food for thought.

On line 745... it is worth to ask a question: "Do we really care what message we get?" Do we run any business logic (decision) based on this error? If yes, this may be a candidate for using sentinel errors, and would require to check what type of the error we get (errors.Is() method from the errors package.

@shaun-nx shaun-nx requested review from jjngx and coolbry95 November 23, 2022 14:43
Copy link
Contributor

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

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

🚀 great! 👍🏻

@shaun-nx shaun-nx enabled auto-merge (squash) November 24, 2022 11:11
@shaun-nx shaun-nx merged commit 169f15f into main Nov 24, 2022
@shaun-nx shaun-nx deleted the endpoint-slices branch November 24, 2022 13:37
@ciarams87 ciarams87 added the enhancement Pull requests for new features/feature enhancements label Jan 11, 2023
@lucacome lucacome added change Pull requests that introduce a change and removed enhancement Pull requests for new features/feature enhancements labels Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Pull requests that introduce a change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants