-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: enrich targets now accepts any scheme #149
Conversation
Signed-off-by: Niklas Treml <treml.niklas@gmail.com>
Signed-off-by: Niklas Treml <treml.niklas@gmail.com>
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.
LGTM but could you add some unit tests for the cases this PR is supposed to fix?
Additionally we should investigate why this test sometimes fails:
=== RUN Test_gitlabTargetManager_Reconcile_Registration_Update
{"time":"2024-06-06T16:34:19.377725874Z","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets.(*manager).Reconcile","file":"/home/runner/work/sparrow/sparrow/pkg/sparrow/targets/manager.go","line":82},"msg":"Starting target manager reconciliation"}
{"time":"2024-06-06T16:34:19.388429797Z","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/test.(*MockClient).PostFile","file":"/home/runner/work/sparrow/sparrow/pkg/sparrow/targets/remote/test/remotemock.go","line":53},"msg":"MockPostFile called","err":null}
{"time":"2024-06-06T16:34:19.478009469Z","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/test.(*MockClient).FetchFiles","file":"/home/runner/work/sparrow/sparrow/pkg/sparrow/targets/remote/test/remotemock.go","line":62},"msg":"MockFetchFiles called","targets":1,"err":null}
{"time":"2024-06-06T16:34:19.478098907Z","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/test.(*MockClient).PutFile","file":"/home/runner/work/sparrow/sparrow/pkg/sparrow/targets/remote/test/remotemock.go","line":44},"msg":"MockPutFile called","err":null}
{"time":"2024-06-06T16:34:19.578621783Z","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/test.(*MockClient).FetchFiles","file":"/home/runner/work/sparrow/sparrow/pkg/sparrow/targets/remote/test/remotemock.go","line":62},"msg":"MockFetchFiles called","targets":1,"err":null}
{"time":"2024-06-06T16:34:19.578713224Z","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/test.(*MockClient).PutFile","file":"/home/runner/work/sparrow/sparrow/pkg/sparrow/targets/remote/test/remotemock.go","line":44},"msg":"MockPutFile called","err":null}
{"time":"2024-06-06T16:34:19.678783951Z","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/test.(*MockClient).FetchFiles","file":"/home/runner/work/sparrow/sparrow/pkg/sparrow/targets/remote/test/remotemock.go","line":62},"msg":"MockFetchFiles called","targets":1,"err":null}
{"time":"2024-06-06T16:34:19.678915467Z","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/test.(*MockClient).PutFile","file":"/home/runner/work/sparrow/sparrow/pkg/sparrow/targets/remote/test/remotemock.go","line":44},"msg":"MockPutFile called","err":null}
manager_test.go:457: Reconcile() should have updated the registration twice
--- FAIL: Test_gitlabTargetManager_Reconcile_Registration_Update (0.30s)
Signed-off-by: Niklas Treml <treml.niklas@gmail.com>
as far as the flaky test goes, I don't have time to look into it, but I'll file a bug report |
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.
LGTM
Created the issue #150 |
Motivation
Closes #148
This improves the logic in
enrichTargets
to account for any scheme, be that http, https, tcp whatever floats your boat, as long as it's a valid url we're happy.Changes
Parse the url into a
net.Url
so we grab the dns name in an easier way for the dns check.For additional information look at the commits.
Tests done
I've tested it with the latency and dns check enabled using mixed set of targets configured with HTTP and HTTPS. Seemed to work fine
TODO