caas-team / sparrow

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

feat: simplify helm chart #75

Closed niklastreml closed 8 months ago

niklastreml commented 8 months ago

Motivation

Simplifies the helm chart by removing the separate startup and runtime configs in favour of having a singular sparrow config file.

Changes

For additional information look at the commits.

Tests done

y-eight commented 8 months ago

My first thought: I dont like that we join the splited config (startup & runtime config), even when using the file loader. This are still different configs. We should prepare the helm chart like this as well.

Second thought: Why not joining, we will just have one config file when e.g. using the file loader

What do you think from user experience point of view? I don't want to confuse joining the config and using different parts of the config by 2 mechanisms... @NiklasTreml @puffitos @lvlcn-t

puffitos commented 8 months ago

We should keep the two configurations separate. How the sparrow is setup to run itself (what is now called Config or StartupConfig) should have almost nothing to do with how the checks are configured (what is now confusingly called RuntimeConfig and we map to an any struct in the code).

Those two configurations should stay separate and be handled in separate ways, starting in the codebase and moving up to the deployment configuration (the helm charts). The sparrow only needs its config to run (the startup config) and if a check configuration file is provided (either via file or via an HTTP endpoint), various checks will also be executed. I'm against providing only one config parameter in the helm chart, as this may seem simpler, but we're effectively combining two non-related things. This will complicate things down the line.

Let's stick with the 2 configs: sparrowConfig and checksConfig. Two separate k8s Objects, two separate values and two separate go structs (and interfaces possibly).

niklastreml commented 8 months ago

The helm chart never had two separate configmaps for the config, but I've update the chart so it now creates one file for each config and loads it into the pod

puffitos commented 8 months ago

The helm chart never had two separate configmaps for the config, but I've update the chart so it now creates one file for each config and loads it into the pod

👍

niklastreml commented 8 months ago

@puffitos @y-eight extra secrets are finally implemented. took a bit longer than expected because I found a bug with parsing environment variables. Ive fixed the bug in 551f07c03a320265cbb4b1b26995ccc928f02972, but I wish it didn't take me 45 minutes to notice...

I did some testing with the helm chart and it seems to work well now

puffitos commented 8 months ago

LGTM (again)