fix: Preserve newline before apiVersion in nico-rest-api configmap#509
Open
jeremypruitt wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix: Preserve newline before apiVersion in nico-rest-api configmap#509jeremypruitt wants to merge 1 commit intoNVIDIA:mainfrom
jeremypruitt wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
The validateAuth include used {{- ... -}} which strips whitespace on
both sides. When validateAuth's success path produces empty output, the
trailing -}} strips the newline before apiVersion: v1, gluing it onto
the end of the SPDX header's final comment line:
# limitations under the License.apiVersion: v1
This produces invalid YAML and the chart fails to install. Removing the
trailing dash preserves the newline.
Verified by rendering with helm template before/after.
Signed-off-by: Jeremy Pruitt <jeremypruitt@me.com>
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
No user-facing changes in this release. WalkthroughThe ConfigMap template in the Helm chart removes the trailing whitespace-trimming operator from the ChangesHelm Template Whitespace Behavior
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
validateAuthinclude at the top ofnico-rest-api/templates/configmap.yamluses{{- ... -}}, which strips whitespace on both sides of the action. WhenvalidateAuthsucceeds (the common case — auth is configured correctly), the helper produces empty output, and the trailing-}}strips the newline that should separate the action fromapiVersion: v1. The result is thatapiVersion: v1gets glued onto the end of the SPDX header's last comment line, producing invalid YAML that breakshelm install.Reproduction
helm template test helm/charts/nico-rest/charts/nico-rest-api \ -s templates/configmap.yaml \ --set global.image.repository=foo --set global.image.tag=v1 --set global.imagePullSecrets= \ --set config.keycloak.enabled=true \ --set config.keycloak.baseURL=https://kc.example.com \ --set config.keycloak.realm=nico \ --set config.keycloak.clientID=nico \ --set config.db.user=nico --set config.db.password=changeme \ --set config.db.host=localhost --set config.db.port=5432 --set config.db.name=nico \ --set secrets.dbCreds=falseBefore (broken)
After (this PR)
Root cause
{{- ... -}}strips all preceding whitespace (including the blank line above the action and the trailing newline of the previous comment) and all trailing whitespace (the newline beforeapiVersion). When the include's success-path output is"", the comment line andapiVersion: v1end up directly concatenated. Replacing-}}with}}preserves the trailing newline.Type of Change
Services Affected
(Helm chart for the API service.)
Related Issues (Optional)
The same one-line change is bundled inside #507 ("Feat/rename ncx to nico") alongside unrelated rename work. Per CONTRIBUTING.md ("Keep pull requests focused on a single change"), this PR extracts the configmap fix so it can land independently.
cc @shayan1995 — your #507 already includes this exact fix; opening this narrower PR so it can land on its own without waiting for the rest of the rename work. Happy to close in favor of #507 if you'd prefer it stay bundled there.
Breaking Changes
Testing
Verified with
helm templatebefore/after on a clean checkout (see Reproduction section). Existing chart unit tests not affected.Additional Notes
The same whitespace-trim pattern (
{{- include ... -}}at the top of a file with text above) does not currently appear in any other template inhelm/charts/nico-rest/, so this is an isolated occurrence.