helm: make terminationGracePeriodSeconds configurable for ingestor and querier#1672
helm: make terminationGracePeriodSeconds configurable for ingestor and querier#1672AdheipSingh wants to merge 2 commits into
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA AdheipSingh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughHelm values add 30s defaults and templates now reference ChangesTermination Grace Period & Chart Metadata
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/templates/querier-statefulset.yaml`:
- Line 40: Replace the brittle chained .Values access with a nil-safe dig
lookup: instead of directly referencing
.Values.parseable.highAvailability.querier.terminationGracePeriodSeconds use the
dig function to safely traverse intermediate maps (e.g. dig .Values "parseable"
"highAvailability" "querier" "terminationGracePeriodSeconds") and then apply
default (| default 30) to provide the fallback; make the same change for the
ingestor terminationGracePeriodSeconds lookup. Ensure you wrap the dig call in
parentheses before piping to default so the default applies to the dig result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 320e7471-ef15-4820-b395-95a8e63552e5
📒 Files selected for processing (4)
helm/Chart.yamlhelm/templates/ingestor-statefulset.yamlhelm/templates/querier-statefulset.yamlhelm/values.yaml
| {{- include "parseable.querierLabelsSelector" . | nindent 8 }} | ||
| spec: | ||
| terminationGracePeriodSeconds: 10 | ||
| terminationGracePeriodSeconds: {{ .Values.parseable.highAvailability.querier.terminationGracePeriodSeconds | default 30 }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Helm templates, does {{ .Values.a.b.c | default 30 }}fail ifbis missing, and isdig the recommended nil-safe pattern?
💡 Result:
Yes, the expression {{ .Values.a.b.c | default 30 }} will fail with a template error if b (or a) is missing [1][2]. Because Helm templates attempt to evaluate the entire path, accessing a property on a nil object causes a nil pointer dereference [1][3]. The dig function is a recommended, nil-safe pattern for accessing deeply nested values [3][4]. It allows you to traverse a dictionary and provide a default value if any key in the chain is missing [3]. Example usage: {{ dig "a" "b" "c" 30.Values }} Key considerations: 1. Digging: dig accepts the target keys as arguments followed by the dictionary (e.g., .Values) [3]. If any key in the sequence a.b.c is missing, it returns the provided default value (30) instead of failing [3][4]. 2. Default Behavior: Be aware that the standard default function in Helm is not just a nil-check; it evaluates whether a value is "empty" based on its type (e.g., empty strings, 0, and false are all considered empty and will trigger the default) [3][5]. 3. Best Practices: While dig is a robust solution for deep nesting, official Helm best practices recommend flattening your configuration structure whenever possible to avoid the need for complex guard clauses or deep-path traversal [1][6]. If you must use nested structures, you can also use if blocks to guard access, though this is more verbose [1][6].
Citations:
- 1: https://stackoverflow.com/questions/59571842/helm-charts-how-do-i-use-default-on-undefined-object-property-values
- 2: Proposal: New omitted function to set a default value if value omitted helm/helm#12863
- 3: https://helm.sh/docs/chart_template_guide/function_list/
- 4: https://github.com/helm/helm-www/blob/main/docs/chart_template_guide/function_list.mdx
- 5: Default function evaluating the default value when the value provided is not omitted helm/helm#12781
- 6: https://helm.sh/docs/chart_best_practices/values/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the template around the referenced line
echo "=== helm/templates/querier-statefulset.yaml (around line 30-55) ==="
nl -ba helm/templates/querier-statefulset.yaml | sed -n '30,60p'
# Inspect values around where the review comment claims the new key exists
echo
echo "=== helm/values.yaml (around line 1-120; narrow to likely region) ==="
if [ -f helm/values.yaml ]; then
nl -ba helm/values.yaml | sed -n '1,140p'
else
echo "helm/values.yaml not found"
fi
# If there are multiple values files, check common ones quickly
echo
echo "=== Search for 'parseable:' in helm values files ==="
ls -la helm || true
fd -t f 'values*.yaml' helm || true
rg -n "parseable:" helm -S || trueRepository: parseablehq/parseable
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== helm/templates/querier-statefulset.yaml (around the terminationGracePeriodSeconds line) ==="
rg -n "terminationGracePeriodSeconds" helm/templates/querier-statefulset.yaml helm/templates -S || true
echo
# Print a small window around the first match in that file (if it exists)
if rg -n "terminationGracePeriodSeconds" helm/templates/querier-statefulset.yaml -S >/dev/null; then
# extract first matching line number
ln=$(rg -n "terminationGracePeriodSeconds" helm/templates/querier-statefulset.yaml -S | head -n1 | cut -d: -f1)
start=$((ln-15)); end=$((ln+15))
if [ "$start" -lt 1 ]; then start=1; fi
sed -n "${start},${end}p" helm/templates/querier-statefulset.yaml
fi
echo
echo "=== Locate parseable.highAvailability.querier values in helm values files ==="
# list likely values files
fd -t f "values*.yaml" helm || true
# print occurrences of the key path
rg -n "parseable\.highAvailability\.querier|highAvailability:\s*\n|highAvailability:" helm -S || true
echo
rg -n "parseable:" helm/values.yaml helm/**/values*.yaml -S || true
echo
# If helm/values.yaml exists, print area around parseable: section
if [ -f helm/values.yaml ]; then
if rg -n "^parseable:" helm/values.yaml -S >/dev/null; then
ln=$(rg -n "^parseable:" helm/values.yaml -S | head -n1 | cut -d: -f1)
start=$((ln-20)); end=$((ln+120))
if [ "$start" -lt 1 ]; then start=1; fi
sed -n "${start},${end}p" helm/values.yaml
else
# fallback: print first 140 lines
sed -n '1,140p' helm/values.yaml
fi
fiRepository: parseablehq/parseable
Length of output: 5657
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== querier-statefulset.yaml: top-level conditionals around highAvailability ==="
sed -n '1,120p' helm/templates/querier-statefulset.yaml
echo
echo "=== Lines mentioning highAvailability.enabled in querier-statefulset.yaml ==="
rg -n "highAvailability\.enabled|highAvailability" helm/templates/querier-statefulset.yaml
echo
echo "=== Check ingestor-statefulset.yaml terminationGracePeriodSeconds chained lookup ==="
rg -n "terminationGracePeriodSeconds" helm/templates/ingestor-statefulset.yaml helm/templates -S
echo
echo "=== Check whether dig is already used in templates ==="
rg -n "\bdig\b" helm/templates -SRepository: parseablehq/parseable
Length of output: 5679
Use a nil-safe dig lookup for terminationGracePeriodSeconds (querier + ingestor).
default won’t protect against missing intermediate maps when rendering {{ .Values.parseable.highAvailability.querier.terminationGracePeriodSeconds | default 30 }} (line 40); if older values set parseable.highAvailability.enabled: true but omit parseable.highAvailability.querier, the template can fail. The same chained pattern exists in helm/templates/ingestor-statefulset.yaml (line 59).
Proposed fix
- terminationGracePeriodSeconds: {{ .Values.parseable.highAvailability.querier.terminationGracePeriodSeconds | default 30 }}
+ terminationGracePeriodSeconds: {{ dig "parseable" "highAvailability" "querier" "terminationGracePeriodSeconds" 30 .Values }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| terminationGracePeriodSeconds: {{ .Values.parseable.highAvailability.querier.terminationGracePeriodSeconds | default 30 }} | |
| terminationGracePeriodSeconds: {{ dig "parseable" "highAvailability" "querier" "terminationGracePeriodSeconds" 30 .Values }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/templates/querier-statefulset.yaml` at line 40, Replace the brittle
chained .Values access with a nil-safe dig lookup: instead of directly
referencing
.Values.parseable.highAvailability.querier.terminationGracePeriodSeconds use the
dig function to safely traverse intermediate maps (e.g. dig .Values "parseable"
"highAvailability" "querier" "terminationGracePeriodSeconds") and then apply
default (| default 30) to provide the fallback; make the same change for the
ingestor terminationGracePeriodSeconds lookup. Ensure you wrap the dig call in
parentheses before piping to default so the default applies to the dig result.
AdheipSingh
force-pushed
the
helm/configurable-termination-grace-period
branch
from
0fcdadb to
d7a4f47
Compare
AdheipSingh
force-pushed
the
helm/configurable-termination-grace-period
branch
from
d7a4f47 to
d2b4c21
Compare
|
closing in favor of #1673 |
2 participants
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit