-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
chore(ci): fix failing test, upgrade to go 1.23.5 and linter to v1.63 #5013
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What was the actual issue? |
@ivankatliarchuk This issue is on recent PRs: See for instance on this one: --- FAIL: TestServiceSource (0.12s)
--- FAIL: TestServiceSource/Endpoints (0.02s)
--- FAIL: TestServiceSource/Endpoints/annotated_services_return_an_endpoint_with_hostname_then_resolve_hostname (0.10s)
service_test.go:1146: Targets expected "23.192.228.84", got "23.192.228.80;23.192.228.84;23.215.0.136;23.215.0.138;96.7.128.175;96.7.128.198"
service_test.go:1146: Targets expected "2600:1406:3a00:21::173e:2e65", got "2600:1406:3a00:21::173e:2e65;2600:1406:3a00:21::173e:2e66;2600:1406:bc00:53::b81e:94c8;2600:1406:bc00:53::b81e:94ce;2600:1408:ec00:36::1736:7f24;2600:1408:ec00:36::1736:7f31"
FAIL |
One of the test use a public domain "example.com", which resolve now to multiple IPs v4 and v6 since a few days. |
Gotcha. I cannot approve but /lgtm ;-) |
@@ -66,6 +66,14 @@ func NewTargets(target ...string) Targets { | |||
return t | |||
} | |||
|
|||
func NewTargetsFromAddr(targets []netip.Addr) Targets { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seems to be only required for tests at the moment. Do you want to keep it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where I should put it.
In the test file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have clarity accross the project then. There is a well RemoveDuplicates
function that only used in single test file endpoints_test.go
, but leave endpoints.go
and is public.
We could
- Leave it as is, it can remain in the endpoint/endpoint.go file, as it deals with operations on Endpoint objects.
- we could have
endpiont/util.go
this will help with codebase organisation a bit - private function in
source/service_test.go
- There is as well
internal/testutils/endpoint.go
that have something liketest utility functions for endpoints verifications
If it's in endpoint.go
file and is public, probably worth to improve it a bit then, in case someone decide to use it production code outside tests
t := make(Targets, len(targets))
for i, target := range targets {
t[i] = target.String()
}
return t
^ avoid repetitive slicing within the loop, taking full advantage of the preallocated capacity.
What does it do ?
Motivation
Latest security fixes and working CI.