nats-io / k8s

NATS on Kubernetes with Helm Charts
Apache License 2.0
444 stars 300 forks source link

Use mergeOverwrite Instead of merge in NATS Helm Chart Templates #913

Closed nikolaigut closed 2 months ago

nikolaigut commented 2 months ago

What motivated this proposal?

I have encountered an issue with the merge function used in the NATS Helm chart templates. The current implementation employs the merge function to combine multiple sources. However, I have observed that if one of the sources changes during an upgrade of the release, the merge function ignores these changes.

Steps to Reproduce:

  1. Deploy a NATS Helm chart using the current configuration with the merge function.
  2. Upgrade the release and modify one of the sources.
  3. Observe that the changes are not reflected due to the merge function.

Expected Behavior: When using mergeOverwrite, any changes in the sources during an upgrade should be acknowledged and applied correctly.

Actual Behavior: Currently, the merge function ignores changes in the sources during an upgrade.

What is the proposed change?

To address this, I propose using the Go template function mergeOverwrite instead of merge. This function should ensure that any changes in the sources are respected during the upgrade process.

Proposed Solution: Replace the merge function with mergeOverwrite in the NATS Helm chart templates.

Comparison of merge and mergeOverwrite

Example files/config/websocket.yaml

{{- with .Values.config.websocket }}
port: {{ .port }}

{{- if .tls.enabled }}
{{- with .tls }}
tls:
  {{- include "nats.loadMergePatch" (mergeOverwrite (dict "file" "config/tls.yaml" "ctx" (mergeOverwrite (dict "tls" .) $)) .) | nindent 2 }}
{{- end }}
{{- else }}
no_tls: true
{{- end }}
{{- end }}

Who benefits from this change?

This change should enhance the flexibility and reliability of the Helm chart upgrade process, ensuring that updates to sources are properly merged.

What alternatives have you evaluated?

No response

caleblloyd commented 2 months ago

However, I have observed that if one of the sources changes during an upgrade of the release, the merge function ignores these changes.

Can you provide an example that recreates that? I don't see how it would change anything with the yaml provided.

nikolaigut commented 2 months ago

You are right. The problem is not in the nats helm chart. The issue is, if the new value is explicitly specified instead of just changing it in the values.yaml of the chart, the change is applied. I apologize for the hastily made issue.