refactor/typed-runtime-cfg

[puffitos]

Motivation

Closes #72

Since we currently have no binding reasons of using any typed configurations if a strongly typed language, we should avoid doing that :) Please read through the text of the PR and come back to it. I've tried to explain everything done.

It's only like 1000 lines of code changed 👍 Although I encourage organizing a call to go over the PR, please go through the changes yourselves; most of the changes are just renamings of imports... The important parts are the interface changes and mostly the ReconcileChecks function of the Sparrow. The rest was just doing due diligence and updating everything so that the code may compile.

Needless to say, to be merged after #91. Also includes most changes from that PR.

Changes

Any type removal

The any Configuration was removed in the following fashion:

• A new struct named Config, that represents the YAML file with the check’s runtime configuration was introduced.
• The Config struct was used everywhere, where the map[any]any appeared.
• The general and abstract character of the any map can’t be expressed directly by a typed Config - this created the need for an interface for those CheckConfigs coming in each time to be able to differentiate between them, without doing type checking each time.
• Additionally, the Checks themselves needed a way to manage their configurations during the ReconcileChecks function of the sparrow. This raised the need for two new interface functions for the Checks themselves, to be able to identify them and manipulate their configs (Name and GetConfig).
• The check’s Config structs must thus implement a new interface, currently named Runtime (used as checks.Runtime - I’m open to better names. The name occurred “organically” from RuntimeConfig, which is the current type being used in the config package). This handles the checks and their configuration during the sparrow’s runtime and reconciliation methods.

The rest was:

• change each Check’s SetConfig and implement the Name and GetConfig
• refactor the sparrow’s methods in run.go - this was the most difficult part, as the ReconcileChecks function is heavy. We should see how we can simplify the logic, if that’s possible.
• fix tests that used the any maps - that was just painful.

Structural

Some moving around of packages had to be done, because of cyclic imports. We should generally avoid creating dedicated packages up until the point we realize, that a package is too big and handles too many responsibilities.

Changes:

• types package is gone, contents were moved to checks into a base.go file, which contains the basic types for the checks and their results.
• errors package was unneeded, can be part of the checks package. Also, naming packages after the stdlib is something we should avoid.
• The register.go file was replaced by an sister-to-checks package named factory, to give more meaning to the existence of this “registry” for checks. A factory to create the checks, given a runtime configuration. The management of which checks are available and their creation shouldn’t be part of the checks package, but a package which uses the checks package.
• A runtime package in checks was introduced, to handle the Runtime Configuration of the checks. It currently cannot be part of the checks package, as the checks themselves need this package to run. Maybe it’s place is in a different package altogether, or maybe we shouldn’t package the checks 🤷

---

You may stop reading here and just start the review. If you need more information though, keep reading on the specific changes.

---

Checks runtime configuration

This struct handles the runtime configuration file for all checks. This struct replaces the any maps with the checks configuration in the whole codebase.

Notable features:

• Every time a check is added, we must update the Config struct as well to handle the extra configuration for the check, or else the check won’t be configurable.
• The struct has some convenience functions which allow getting the whether the config is empty, whether checks exist and iterating over the checks’ configurations. Those must also be updated for each new check and are mostly used in the already present functions.

The runtime configuration of the checks was removed from the startup configuration check struct (in the config package), as this was the wrong place for it. Finally, the unused and currently unneeded apiVersion and kind fields of the configuration were removed from the configuration file. RIP (not).

Still open (for the future):

• The Interface should also have a Validate method to validate the configuration per check. Left it out, as this would only enlarge this already large enough PR.
• The Validate can then trickle up to the runtime.Config struct, which should have a validate Method to validate all configurations.

Check Interface & Check handling

The Check interface was modified, as it was the most sensible thing that came in mind to be able to handle the update of the checks themselves during the ReconcileChecks routine. The interface was thus expanded:

• to return the Name of the Check, which allows the identification of the check
• to be able to get the current Config of the check, to handle the update of the configuration of the current checks. This seems a bit weird, maybe we should fix this.

To create new checks, a proper Factory was created to just call the constructors and also create multiple checks from the incoming runtime.Config from the configuration channel of the Sparrow.

Tests done

• Unit tests run
• Simple E2E tests run (we must run E2E tests ASAP)

Final remarks

Most fragile changes of this refactoring were made because of how the ReconcileChecks works, like expanding the Checks interface with a GetConfig, which seems unnecessary to just update the config of a check. We should focus some time on finding a simpler logic on how to CRUD the checks managed by the sparrow, instead of managing them in a map[string]checks.Check (a map of names to interfaces).

[lvlcn-t]

@puffitos Can you create an issue to the validation part you've mentioned in your PR?

[y-eight]

Regarding the Runtime interface name, no better idea for now. Seems ok for me for now!

[y-eight]

cfg.Empty validation here seems a bit smelly since we are starting all checks if none is configured. (In the second reconciliation run though, targets will be enriched since the config is not empty anymore).

We might want to have a look at the reconciliation logic in a dedicated PR anyway, so this is not that important.

[puffitos]

Since there currently is no validation (outside the YAML syntax one) when grabbing a new runtime.Config, the following config may also be pushed to the channel:

incoming := runtime.Config{}

// Parsed from an empty YAML file, which is a legit option

We should either stop something from this ever happening at loader-time, or just write a simple guard here, which makes more sense to me.

The point about reworking the function is valid.

[lvlcn-t]

LGTM but can you open the validation issue I mentioned before and maybe an issue to your final remark regarding the mapping of check names to interfaces.