search

caas-team / sparrow

A monitoring tool to gather infrastructure network information
Apache License 2.0
7 stars 4 forks source link

feat: add configurable branches for gitlab target manager #182

Closed lvlcn-t closed 2 months ago

lvlcn-t commented 2 months ago

Motivation

Closes #179

Changes

I've added a configuration option to let the user set the branch to use.

Additional to that, I've also improved error handling of response body close errors and refactored the gitlab interactor to use the (slog.Logger).Context methods.

For additional information look at the commits.

Tests done

Manual E2E tests

Registration Repository

Branch set to main

Logs:

❯ go run . run --config .tmp/config/start-config.yaml
Using config file: .tmp/config/start-config.yaml
{"time":"2024-09-05T14:28:18.142951065+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/cmd.NewCmdRun.run.func1","file":"/home/installadm/dev/github/sparrow/cmd/run.go","line":89},"msg":"Running sparrow"}
{"time":"2024-09-05T14:28:18.143364199+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets.(*manager).Reconcile","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/manager.go","line":82},"msg":"Starting target manager reconciliation"}
{"time":"2024-09-05T14:28:18.143551298+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/api.(*api).Run.func1","file":"/home/installadm/dev/github/sparrow/pkg/api/api.go","line":101},"msg":"Serving Api","addr":":8080"}
{"time":"2024-09-05T14:28:18.144054588+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/installadm/dev/github/sparrow/pkg/config/file.go","line":71},"msg":"File Loader disabled"}
{"time":"2024-09-05T14:29:18.50445464+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/gitlab.(*client).FetchFiles","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/remote/gitlab/gitlab.go","line":91},"msg":"Successfully fetched all target files","files":2}
{"time":"2024-09-05T14:30:19.010927357+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/gitlab.(*client).FetchFiles","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/remote/gitlab/gitlab.go","line":91},"msg":"Successfully fetched all target files","files":3}
^C{"time":"2024-09-05T14:30:36.156132844+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/cmd.NewCmdRun.run.func1","file":"/home/installadm/dev/github/sparrow/cmd/run.go","line":96},"msg":"Signal received, shutting down"}
{"time":"2024-09-05T14:30:36.156201124+02:00","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets.(*manager).Reconcile","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/manager.go","line":86},"msg":"Error while reconciling targets","err":"context canceled"}
{"time":"2024-09-05T14:30:36.156204741+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow.(*Sparrow).shutdown.func1","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/run.go","line":184},"msg":"Shutting down sparrow gracefully"}
{"time":"2024-09-05T14:30:36.715785242+02:00","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/api.(*api).Run.func1","file":"/home/installadm/dev/github/sparrow/pkg/api/api.go","line":110},"msg":"Failed to serve api","error":"http: Server closed","scheme":"http"}
{"time":"2024-09-05T14:30:36.715881332+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow.(*ChecksController).Shutdown","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/controller.go","line":84},"msg":"Shutting down checks controller"}

Repository commits: image

Branch set to registry

Logs:

❯ go run . run --config .tmp/config/start-config.yaml
Using config file: .tmp/config/start-config.yaml
{"time":"2024-09-05T14:41:58.640940655+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/cmd.NewCmdRun.run.func1","file":"/home/installadm/dev/github/sparrow/cmd/run.go","line":89},"msg":"Running sparrow"}
{"time":"2024-09-05T14:41:58.641682887+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/api.(*api).Run.func1","file":"/home/installadm/dev/github/sparrow/pkg/api/api.go","line":101},"msg":"Serving Api","addr":":8080"}
{"time":"2024-09-05T14:41:58.641874829+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets.(*manager).Reconcile","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/manager.go","line":82},"msg":"Starting target manager reconciliation"}
{"time":"2024-09-05T14:41:58.642649172+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/installadm/dev/github/sparrow/pkg/config/file.go","line":71},"msg":"File Loader disabled"}
{"time":"2024-09-05T14:42:58.995044136+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/gitlab.(*client).FetchFiles","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/remote/gitlab/gitlab.go","line":91},"msg":"Successfully fetched all target files","files":2}
{"time":"2024-09-05T14:43:59.403038574+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/gitlab.(*client).FetchFiles","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/remote/gitlab/gitlab.go","line":91},"msg":"Successfully fetched all target files","files":3}
{"time":"2024-09-05T14:44:59.78347422+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/gitlab.(*client).FetchFiles","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/remote/gitlab/gitlab.go","line":91},"msg":"Successfully fetched all target files","files":3}
^C{"time":"2024-09-05T14:45:14.175172839+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/cmd.NewCmdRun.run.func1","file":"/home/installadm/dev/github/sparrow/cmd/run.go","line":96},"msg":"Signal received, shutting down"}
{"time":"2024-09-05T14:45:14.175247979+02:00","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets.(*manager).Reconcile","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/manager.go","line":86},"msg":"Error while reconciling targets","err":"context canceled"}
{"time":"2024-09-05T14:45:14.175255601+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow.(*Sparrow).shutdown.func1","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/run.go","line":184},"msg":"Shutting down sparrow gracefully"}
{"time":"2024-09-05T14:45:14.588361399+02:00","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/api.(*api).Run.func1","file":"/home/installadm/dev/github/sparrow/pkg/api/api.go","line":110},"msg":"Failed to serve api","error":"http: Server closed","scheme":"http"}
{"time":"2024-09-05T14:45:14.58844081+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow.(*ChecksController).Shutdown","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/controller.go","line":84},"msg":"Shutting down checks controller"}

Repository commits: image

No branch configured (defaults to the default branch of the repository)

Logs:

❯ go run . run --config .tmp/config/start-config.yaml
Using config file: .tmp/config/start-config.yaml
{"time":"2024-09-05T14:47:45.420429551+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/gitlab.(*client).fetchDefaultBranch","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/remote/gitlab/gitlab.go","line":414},"msg":"Successfully fetched default branch","gitlab":"https://gitlab.devops.telekom.de","branch":"main"}
{"time":"2024-09-05T14:47:45.420514875+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/cmd.NewCmdRun.run.func1","file":"/home/installadm/dev/github/sparrow/cmd/run.go","line":89},"msg":"Running sparrow"}
{"time":"2024-09-05T14:47:45.420764869+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets.(*manager).Reconcile","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/manager.go","line":82},"msg":"Starting target manager reconciliation"}
{"time":"2024-09-05T14:47:45.420974446+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/installadm/dev/github/sparrow/pkg/config/file.go","line":71},"msg":"File Loader disabled"}
{"time":"2024-09-05T14:47:45.421105583+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/api.(*api).Run.func1","file":"/home/installadm/dev/github/sparrow/pkg/api/api.go","line":101},"msg":"Serving Api","addr":":8080"}
{"time":"2024-09-05T14:48:45.756452477+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets/remote/gitlab.(*client).FetchFiles","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/remote/gitlab/gitlab.go","line":91},"msg":"Successfully fetched all target files","files":2}
^C{"time":"2024-09-05T14:49:06.789190618+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/cmd.NewCmdRun.run.func1","file":"/home/installadm/dev/github/sparrow/cmd/run.go","line":96},"msg":"Signal received, shutting down"}
{"time":"2024-09-05T14:49:06.789319656+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow.(*Sparrow).shutdown.func1","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/run.go","line":184},"msg":"Shutting down sparrow gracefully"}
{"time":"2024-09-05T14:49:06.789350574+02:00","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets.(*manager).Reconcile","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/targets/manager.go","line":86},"msg":"Error while reconciling targets","err":"context canceled"}
{"time":"2024-09-05T14:49:07.291671759+02:00","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/api.(*api).Run.func1","file":"/home/installadm/dev/github/sparrow/pkg/api/api.go","line":110},"msg":"Failed to serve api","error":"http: Server closed","scheme":"http"}
{"time":"2024-09-05T14:49:07.2917532+02:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow.(*ChecksController).Shutdown","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/controller.go","line":84},"msg":"Shutting down checks controller"}
{"time":"2024-09-05T14:49:07.29179145+02:00","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow.(*Sparrow).Run","file":"/home/installadm/dev/github/sparrow/pkg/sparrow/run.go","line":130},"msg":"Non-recoverable error in sparrow component","error":"context canceled"}

Repository commits: image

TODO

lvlcn-t commented 2 months ago

Looks OK, but some things should change as they introduce some complexity we don't need.

I think the biggest problem IMO here is, that we're using the interface in the target manager. The target manager should just have a config; this would have been the gitlab.Config now. Then there would be no need to change the Bytes() to Serialize(branch string), as the target manager would know which branch it's supposed to talk to.

We introduced the abstraction way too soo with no concrete use case. Please let's stop doing this; we've already done a major cleanup on the code base almost a year ago, because it had many premature abstractions which got in the way of our requirements.

Yeah I've also thought about this and maybe we should move some of the additional attributes of the remote.File type. There are some implementation details that should not concern the target manager. The target manager should only take care about the file name and the content of the file. The rest of the attributes should be handled by the remote.Interactor implementation, as it is the one that knows how to interact with the registration service, regardless whether it's GitLab or any other service which we might add in the future as an implementation of the remote.Interactor interface. What do you think about this?

puffitos commented 2 months ago

Yeah I've also thought about this and maybe we should move some of the additional attributes of the remote.File type. There are some implementation details that should not concern the target manager. The target manager should only take care about the file name and the content of the file. The rest of the attributes should be handled by the remote.Interactor implementation, as it is the one that knows how to interact with the registration service, regardless whether it's GitLab or any other service which we might add in the future as an implementation of the remote.Interactor interface. What do you think about this?

@lvlcn-t

The target manager should only take care about the file name and the content of the file.

That's exactly what it should do, well said. The TargetManager interface was built with a Git Remote backend in mind but was kept also as generic as possible - and we should try and keep it that way at first. The Interactor interface tries to add an even more generic spin on an already high-level interface.

I'm afraid that's that exactly the problem: the added layer of abstraction with the Interactor. Each TargetManager can have its own logic & abstraction on how to interact with the remote (or local) backend and its files. The possibilities are too numerous to count. There's currently no hard requirement (outside of helping us be more generic; as you've noted in your https://github.com/caas-team/sparrow/pull/122) for this feature.

I suggest we discuss this in a smaller circle with @y-eight the next couple of days and see how to move on from here. Since we're mostly expecting people to primarily use Gitlab/ Github to registrer their targets, we should also strive to achieve to facilitate this. I'm pretty sure two different implementations of a TargetManager would be more than enough.