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

Gitlab Target Reconciliation #44

Merged
merged 32 commits into from
Dec 18, 2023
Merged

Gitlab Target Reconciliation #44

merged 32 commits into from
Dec 18, 2023

Conversation

puffitos
Copy link
Collaborator

@puffitos puffitos commented Dec 14, 2023

Motivation

Addresses #30.

Changes

This PR introduces two new Interfaces:

  • TargetManager: this serves as a handler for the GlobalTargets struct. It is responsible for updating the local globalTargets state of the Sparrow, by periodically getting the new targets and also registering the Sparrow as a GlobalTarget in the configured remote backend. The TargetManager should also be able to return the current state (AKA the list of GlobalTargets).

  • Gitlab: this is an Interface to handle the interaction with the Gitlab API (mostly the Files API of Gitlab). Its responsiblities are to get the current globalTargets from a remote gitlab repository and update the file containing the registration of the Sparrow itself. The next step is implementing a DeleteFile function to handle de-registration.

The implementation of those interfaces handle the interaction of the Sparrow with the remote Gitlab repository holding all the targets.

Tests done

  • A buttload of unittests for almost all functions.
  • I've tested the file creation and update via cURL, no real E2E tests done yet, to see if the sparrow works.

Concerns

We still need to handle where the config for the TargetManager should be parsed. Currently, we need the base gitlab URL, the project ID (could be merged into one variable) and the gitlab token to access the repo. Also, some durations:

  • check interval for new targets
  • registration interval (how often to update own registration)
  • unhealthy interval (when to ignore a global target - maybe we could even remove this and "dumb down" our Sparrow.

The PutFile, PostFile and FetchFiles functions are similar, which introduce some code duplication. As those functions won't need to be updated significantly for the foreseeable future, we can just refactor them some time down the line to reduce the codebase and simplify testing and usage of the Gitlab interface. A solution would be to use a builder Pattern (`Gitlab().Files().List(), Gitlab().Files().Get("filename") and so on), a la k8s go-client.

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly
  • Build and run the sparrow locally to test whether it all really works :)

Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@puffitos puffitos added the request/internal Indicates an internal feature request label Dec 14, 2023
@puffitos puffitos self-assigned this Dec 14, 2023
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@puffitos
Copy link
Collaborator Author

puffitos commented Dec 14, 2023

1300 lines added. It's all tests and some repetitive and hopefully easy to read code :)

@niklastreml
Copy link
Collaborator

LGTM, as discussed I'm going to ignore the hardcoded values, since config and docs will be added in a separate pr

- feat: marking sparrow as registered after found own URL in targets
- fix: encoding of File's Content field for API
- chore: codebase cleanup, logs & linting
@puffitos
Copy link
Collaborator Author

Did manual E2E testing. The instance can register and update itself:

The sparrow is also sucessfully grabing the global targets (it-self included) from Gitlab:

{"time":"2023-12-15T15:23:48.106570143+01:00","level":"INFO","msg":"Successfully fetched all target files","sparrow":{"files":3}}
{"time":"2023-12-15T15:23:58.550772143+01:00","level":"INFO","msg":"Successfully fetched all target files","sparrow":{"files":3}}
{"time":"2023-12-15T15:24:09.118347133+01:00","level":"INFO","msg":"Successfully fetched all target files","sparrow":{"files":3}}
{"time":"2023-12-15T15:24:12.413958783+01:00","level":"INFO","msg":"Successfully updated registration"}

Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Copy link
Collaborator

@lvlcn-t lvlcn-t left a comment

Choose a reason for hiding this comment

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

Just a few notes here and there, but LGTM overall. Great job!

pkg/sparrow/run.go Outdated Show resolved Hide resolved
pkg/sparrow/targets/gitlab.go Outdated Show resolved Hide resolved
pkg/sparrow/targets/targetmanager.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
Copy link
Member

@y-eight y-eight left a comment

Choose a reason for hiding this comment

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

First review

pkg/sparrow/run.go Outdated Show resolved Hide resolved
pkg/sparrow/run.go Outdated Show resolved Hide resolved
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Copy link
Member

@y-eight y-eight left a comment

Choose a reason for hiding this comment

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

Well done @puffitos. Readability is on point

cmd/run.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
@@ -64,6 +65,10 @@ func New(cfg *config.Config) *Sparrow {
router: chi.NewRouter(),
}

// Set the target manager
gm := targets.NewGitlabManager("sparrow-with-cfg-file", cfg.TargetManager)
Copy link
Member

Choose a reason for hiding this comment

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

The name (domain name) needs to be configurable as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will resolve in separate Issue

pkg/sparrow/targets/gitlab.go Outdated Show resolved Hide resolved
pkg/sparrow/gitlab/gitlab.go Outdated Show resolved Hide resolved
pkg/sparrow/gitlab/gitlab.go Show resolved Hide resolved
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Copy link
Member

@y-eight y-eight left a comment

Choose a reason for hiding this comment

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

LGTM as discussed

@puffitos puffitos merged commit 241ca70 into main Dec 18, 2023
7 checks passed
@puffitos puffitos deleted the feat/registration-config branch December 18, 2023 15:19
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request/internal Indicates an internal feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants