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

helm: Avoid error in IDE due to .range keyword #25766

Merged
merged 1 commit into from May 31, 2023

Conversation

sayboras
Copy link
Member

Description

As range is a keyword in jinja template, using the expression with usual attribute name (e.g. .Values.nodePort.range) will cause the error in IDE. This issue makes it hard for deployment helm for normal operation like checking where the flag is used.

This commit is to use alternative way to get around the issue with get function.

Testing was done as per below

$ cat temp_values.yaml
nodePort:
  range: "30000,32767"
$ helm template cilium install/kubernetes/cilium -f temp_values.yaml | grep node-port-range
  node-port-range: "30000,32767"
  enable-auto-protect-node-port-range: "true"

image

As range is a keyword in jinja template, using the expression with usual
attribute name (e.g. .Values.nodePort.range) will cause the error in IDE.
This issue makes it hard for deployment helm for normal operation like
checking where the flag is used.

This commit is to use alternative way to get around the issue with get
function.

Testing was done as per below

```
$ cat temp_values.yaml
nodePort:
  range: "30000,32767"
$ helm template cilium install/kubernetes/cilium -f temp_values.yaml | grep node-port-range
  node-port-range: "30000,32767"
  enable-auto-protect-node-port-range: "true"
```

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 30, 2023
@sayboras sayboras added the release-note/misc This PR makes changes that have no direct user impact. label May 30, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 30, 2023
@sayboras sayboras marked this pull request as ready for review May 30, 2023 14:33
@sayboras sayboras requested review from a team as code owners May 30, 2023 14:33
@sayboras sayboras requested review from youngnick and kaworu May 30, 2023 14:33
@sayboras sayboras added the area/helm Impacts helm charts and user deployment experience label May 30, 2023
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥷

@sayboras
Copy link
Member Author

/test

@michi-covalent michi-covalent merged commit 1c0e769 into cilium:main May 31, 2023
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants