Repository navigation
[nats] Fixes for NATS App Helm chart, fix template issues with config.merge - #1583
Timofei Larkin (lllamnyp) merged 2 commits into
Conversation
…value Signed-off-by: Isaiah Olson <[email protected]>
WalkthroughUpdates a single Helm template to simplify and broaden conditionals that control rendering of merged config, resolver, and credentials blocks in the NATS manifest; shifts to with-template rendering and reduces indentation levels. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Helm as Helm engine
participant Values as .Values
participant Template as nats.yaml template
participant Manifest as Rendered manifest
Helm->>Template: Render nats.yaml with .Values
Template->>Values: check if .Values.config.merge exists OR passwords present
alt merge condition true
Template->>Manifest: render merged config (with .Values.config.merge)
else merge condition false
Template-->>Manifest: skip merged config
end
Template->>Values: check for resolver block
alt resolver exists
Template->>Manifest: render resolver (with .Values.config.resolver)
else
Template-->>Manifest: skip resolver
end
Template->>Values: check for any password entries
alt passwords present
Template->>Manifest: render accounts/credentials sections using passwords
else
Template-->>Manifest: skip accounts/credentials rendering
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello Isaiah Olson (@insignia96), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two critical bugs within the NATS application Helm chart that previously hindered successful deployments, particularly when user authentication was combined with custom configurations. The changes ensure that NATS deployments can now properly utilize both user authentication and custom settings without encountering YAML parsing errors or incorrect configuration nesting, thereby enabling more robust and flexible NATS deployments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses two critical bugs in the NATS Helm chart that prevented deployments with user authentication and custom configurations. The fixes correctly add existence checks for .Values.config.merge to prevent YAML parsing errors and adjust the indentation to ensure custom configurations are merged at the correct level. The changes are well-implemented. I've added one suggestion to refactor a repeated condition into a variable to improve code clarity and maintainability.
Andrei Kvapil (kvaps)
added
the
kind/backport
|
Hey Isaiah Olson (@insignia96) many thanks for your contribution! Could you please confirm that it won't conflict with this change? |
| {{- if and .Values.config (hasKey .Values.config "merge") (gt (len .Values.config.merge) 0) }} | ||
| {{ toYaml .Values.config.merge | nindent 10 }} |
There was a problem hiding this comment.
| {{- if and .Values.config (hasKey .Values.config "merge") (gt (len .Values.config.merge) 0) }} | |
| {{ toYaml .Values.config.merge | nindent 10 }} | |
| {{- with .Values.config.merge }} | |
| {{- toYaml . | nindent 10 }} |
There isn't a need to check if .Values.config.merge is set (it's at least an empty dict), but the with block will not render if it is empty, fixing the issue. Good catch with the indent, however.
| fullnameOverride: {{ .Release.Name }} | ||
| config: | ||
| {{- if or (gt (len $passwords) 0) (gt (len .Values.config.merge) 0) }} | ||
| {{- if or (gt (len $passwords) 0) (and .Values.config (hasKey .Values.config "merge") (gt (len .Values.config.merge) 0)) }} |
There was a problem hiding this comment.
| {{- if or (gt (len $passwords) 0) (and .Values.config (hasKey .Values.config "merge") (gt (len .Values.config.merge) 0)) }} | |
| {{- if or $passwords .Values.config.merge }} |
Same as my previous comment, no need to check the parent keys, they are defined by default. Also empty dicts are falsey values, so this check could be more concise.
| @@ -63,8 +63,8 @@ spec: | |||
| password: "{{ $password }}" | |||
| {{- end }} | |||
| {{- end }} | |||
There was a problem hiding this comment.
This is outside the diff range, but in the same gist as all the other suggestions, you could apply a refactor like this:
| {{- with $passwords }} | |
| accounts: | |
| A: | |
| users: | |
| {{- range $username, $password := . }} | |
| - user: "{{ $username }}" | |
| password: "{{ $password }}" | |
| {{- end }} | |
| {{- end }} |
This patch makes the fixes from `b1ebc9cc` by @insignia96 terser by making use of Helm's `with` blocks. Signed-off-by: Timofei Larkin <[email protected]>
dosubot
Bot
added
size/S
|
Successfully created backport PR for |
3 participants
[nats] Fixes for NATS App Helm chart, fix template issues with config.merge value
What this PR does
This PR fixes two critical bugs in the NATS application chart (
packages/apps/nats) that prevented successful deployment when using user authentication with custom configuration:Bug #1: YAML Parse Error with Empty config.merge
config.mergewas empty ({}), the Helm template would fail with YAML parse errors.Values.config.mergeand preventing rendering of empty merge blocksBug #2: Incorrect Config Nesting
usersandconfig.mergewere defined, custom NATS config values (e.g.,max_payload,max_connections) were incorrectly nested as children of theaccountsobject instead of being placed at the root NATS configuration levelnindent 12tonindent 10in the templateThe related issue is #1354
Changes Made
File:
packages/apps/nats/templates/nats.yaml.Values.config.mergeImpact
Testing
Can be tested with:
Expected result: HelmRelease succeeds, NATS pods start successfully, and configuration is valid.
Release note
Summary by CodeRabbit
Bug Fixes
Chores