Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

agent merge logic does not properly handle booleans #35

Closed
wmcdona89 opened this issue Sep 7, 2020 · 2 comments
Closed

agent merge logic does not properly handle booleans #35

wmcdona89 opened this issue Sep 7, 2020 · 2 comments
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@wmcdona89
Copy link
Contributor

the merge function does not properly handle booleans as true overwrites false - see helm issue 7313

merge

Merge two or more dictionaries into one, giving precedence to the dest dictionary:

the chart uses merge to merge agent into additionalAgents to ensure they at least have the default values

the following boolean values are impacted:
agent.privileged
agent.alwaysPullImage
agent.TTYEnabled

test template

{{- range $name, $additionalAgent := .Values.additionalAgents }}
  {{- $additionalAgent := merge $additionalAgent $.Values.agent }}
  {{- $name }}:
  {{- toYaml $additionalAgent | nindent 2 }}
{{- end }}

given:

values.yaml

agent:
  alwaysPullImage: true
  privileged: true
  TTYEnabled: true
additionalAgents:
  maven:
    alwaysPullImage: false
    privileged: false
    TTYEnabled: false

expected result:

maven:
  alwaysPullImage: false
  privileged: false
  TTYEnabled: false

actual result:

maven:
  alwaysPullImage: true
  privileged: true
  TTYEnabled: true

Fix

using mergeOverwrite with deepCopy resolves the issue...but requires helm v2.16.0 and above
deepCopy was added in sprig v2.22 which was introduced in helm v2.16.0
mergeOverwrite was added in sprig v2.18 which was introduced in helm v2.13.0

mergeOverwrite

Merge two or more dictionaries into one, giving precedence from right to left, effectively overwriting values in the dest dictionary. This is a deep merge operation but not a deep copy operation. Nested objects that are merged are the same instance on both dicts. If you want a deep copy along with the merge than use the deepCopy function along with merging.

deepCopy

The deepCopy function takes a value and makes a deep copy of the value. This includes dicts and other structures.

test template

{{- range $name, $additionalAgent := .Values.additionalAgents }}
  {{- $additionalAgent := mergeOverwrite (deepCopy $.Values.agent) $additionalAgent }}
  {{- $name }}:
  {{- toYaml $additionalAgent | nindent 2 }}
{{- end }}

given:

values.yaml

agent:
  alwaysPullImage: true
  privileged: true
  TTYEnabled: true
additionalAgents:
  maven:
    alwaysPullImage: false
    privileged: false
    TTYEnabled: false

expected result:

maven:
  alwaysPullImage: false
  privileged: false
  TTYEnabled: false

actual result:

maven:
  alwaysPullImage: false
  privileged: false
  TTYEnabled: false
@stale
Copy link

stale bot commented Oct 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 10, 2020
@stale
Copy link

stale bot commented Oct 24, 2020

This issue is being automatically closed due to inactivity.

@stale stale bot closed this as completed Oct 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

1 participant