Issue 159: Add common.label and common.annotations Helm parameters

[Viktorsubota]

Thanks for submitting a PR! Please check the boxes below:

• I have filled in the "Changes" section below?
• I have filled in the "How did you test this code" section below?
• I have bumped the version number in /charts/flagsmith/Chart.yaml in the section version or I'm merging to a
  release branch

Changes

Features

• Adding commonLabels and commonAnnotations field to the values file
• Updating flagsmith.labels to use commonLabels if provided
• Adding flagsmith.annotations to use commonannotations if provided
• Adding annotations to places where they were missed
• Update existing annotations to add commonAnnotations as well

Fixes

• Fix wrong annotations used in service-fronend.yaml file

How did you test this code?

I ran helm install with different parameters to verify that new labels and annotations are added only if needed.

[matthewelwell]

I'm a little bit unsure what's happening here. It may be that I've exhausted my helm knowledge but I'm unsure where the "context" key fits in here? Is this something that is standard in helm to pass from a parent chart perhaps?

[Viktorsubota]

This piece of code is designed to include annotations from the commonAnnotations section in the values file. If the "context" parameter is not set to the default value (.), but instead is a map containing two keys (customAnnotations and context), the code includes both the provided annotations and those from commonAnnotations. This flexibility is useful when different sets of values need to be passed.

The "context" key in this code is not a standard Helm feature for passing values. It's used here as a parameter to conditionally determine whether to include additional annotations. For instance, when you have different values for specific resources, such as .Values.ingress.api.annotations for ingress resources or .Values.service.api.annotations for services.

Helm/Go templates have some limitations in terms of flexibility, and this code is a workaround to handle different annotation scenarios.

A similar approach can be found in Bitnami Helm Charts, here.

[Viktorsubota]

Hey @matthewelwell, any updates on this?

[matthewelwell]

Hi @Viktorsubota, apologies for the delay here, I've been a bit swamped with other responsibilities.

Given your explanation here, I would expect to see some reference to the context in the docs changes you added in the PR here Flagsmith/flagsmith#3303 but perhaps I'm still misunderstanding? Are you able to provide some kind of example snippet from a values.yaml where the context attribute is used?

[Viktorsubota]

Hello @matthewelwell ,

Let me explain a bit differently.

I added commonAnnotations and commonLabels to be attached to all resources crated by the chart. But there is a collision with the resources "annotated" with their specific annotations.

So the idea here is to pass existing annotations to the template so It can render both if anythig is passed, or just commonAnnotaions.

Here is an example for the frontend service resource:

 {{- $annotations := include "flagsmith.annotations" ( dict "customAnnotations" .Values.service.frontend.annotations "context" . ) }}

What's exactly happening here is flagsmith.annotations is called with a dict parameter (Helm doesn't support passing multiple parameters here) with 2 keys:

• customAnnotations: Resource-specific annotations.
• context: The parameter which we usually pass to the include function. It's needed to access .Values.commonAnnotations within the template.

The logic in flagsmith.annotations checks if . is a dict with those parameters to render both. Or renders just commonAnnotations from . (we pass . by default).

Note, regardless of what you pass to include, the parameter name is always . within a template, and . as an argument is not equal to . as a parameter inside the template.

To summarize:
There were a few places where annotations were already used. In order to avoid duplicating templates, I added some logic to receive and render these annotations alongside commonAnnotations.

Please let me know if this addresses all your questions.
Also, I'm planning to update the documentation PR to include this.

Thanks!

[Viktorsubota]

Oh, I just looked through the Helm doc in the main repo one more time. I don't think including information about context and other internals of the chart would be beneficial there.

Adding annotations to resources is a one-time afford done by devs, but the docs are focused on use by users.

If you believe including information about the implementation would help, let me know, and I will update the doc.

[khvn26]

Hey @Viktorsubota, thank you for the explanation. I think we, with your help, understand the nature of the introduced changes but, IMO, there's room for improvement in the naming and values files structure:

1. Let's have a common key in the values file to house the common attributes, like so:

common:
    annotations:
    labels:

This way we're getting the following improvements:

• Common naming convention for per-service and common annotations and labels, and a clear path for extending the chart with more common attributes in the future.
• We won't have to assign the whole values structure to the context variable for the concatenation logic to work.

2. I believe the dict concatenating code will make more sense if we rename customAnnotations to perResourceAnnotations, and assign only .Values.common to context which probably also can be named better (e.g. commonValues).

[khvn26]

Additionally, we need to take care of:

• The currently failing linting
• Rebasing/merging in the main branch

[Viktorsubota]

@matthewelwell , @khvn26 , hey guys, sorry for a late fix - a lot of work.
All fixed, please take a look.

[khvn26]

LGTM 👍