Refactor and Document chart values

[5nafu]

Hi @mstein11,

As the offered PR for the additional findings of #26 might get a bit bigger, I create this issue for discussion.

As a user of the docker-hub-rate-limit-exporter, I want to see all available variables and their defaults in the values.yaml file for a better overview of possible configurations.

[5nafu]

While writing the update, I see that there is some inconsistency in the usage of some variables:

• daemonset vs. deployment
  • some variables are migrated directly into the different configurations (like daemonset.affinity or daemonset.nodeSelector) while other remain in the top level like podSecurityContext or hostNetwork.
  • as I would guess, that one would either use a deployment or daemonset, I would suggest to revert these variables to the top level.
  • this change was introduced in feat: enhance chart features, add daemonset, add linting ci #24
  • this will probably be a breaking change
• image and pullsecret
  • all variables controlling the image are consolidated under the image variable
  • except the imagePullSecrets
  • I would suggest to migrate immagePullSecrets into the image.immagePullSecrets
  • this will probably be a breaking change

[mstein11]

Hi @5nafu,

thank you for contributing, I like your ideas! From here on @MaxStroh is going to take over from viadee side. He has some ideas of how we could implement the suggested changes.

Best,
Marius

[MaxStroh]

Hi @5nafu,

thanks for your input. We discussed this shortly and want to make our thoughts about this transparent to you.

First of all, we totally agree, that the values referenced in the templates should be consistent with the arrangement of the variables in the values.yaml. Therefore we should definitely fix the references of daemonset.affinity and daemonset.nodeSelector in the daemonset template (and the corresponding references in the deployment template) back to the toplevel variables, which can already be set in the values.yaml on toplevel. This would probably not even cause breaking changes because these references in the templates are simply not working now (except users explicitly added daemonset.affinity e.g. in their own values.yaml file).

Regarding the variable imagePullSecrets we would prefer sticking to the defaults that helm delivers. As most users probably create helm charts via helm create ... we should also stick to the structure, in which helm itself structures these variables. If you run helm create ... you will get a chart like this:

image:
  repository: nginx
  pullPolicy: IfNotPresent
  tag: ""

imagePullSecrets: []
...

With regard to the principle of least astonishment, restructuring these variables other than the helm default, we fear to confuse most of the users - although I agree, that I do not fully understand why helm puts e.g. pullPolicy under image, but imagePullSecrets not. I assume that they also stick to the default on which hierarchy level the values are later parsed into the yaml of a e.g. deployment yaml.

Long story short:

• We would prefer to leave imagePullSecrets as toplevel variable
• We would prefer to fix the references in the template yamls from daemonset.affinity / daemonset.nodeSelector / deployment.affinity / deployment.nodeSelector to toplevel affinity / nodeSelector

Regarding the potentially breaking changes this generates, we think there are three ways to handle this:

1. We simply change these references in the template yamls, causing breaking changes for users who upgrade the charts (maybe automatically), causing the dashboards to simply not work anymore without a concrete hint about why
2. We allow both kinds of value structuring, causing no breaking changes, but confusing new users which way to use and increasing the maintenance
3. We change the references in the template yamls but additionally implement that a speaking and detailed error message is thrown, including which variables are set wrong and where they should be defined now. Although this also causes breaking changes for users, it informs them exactly why it does not work anymore

We would prefer option 3 - what do you think about how to handle breaking changes and our conclusion about the variable structuring?

[5nafu]

Cool. I'll have a look at it over the weekend. Talk to you soon.