fix(self-managed): consume reconnect-enabled SIS chart#392
Conversation
Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe self-managed Helmfile updates the ChangesSIS chart update
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 `@deploy/stacks/self-managed/helmfile.d/02-core.yaml.gotmpl`:
- Line 42: Add validation for the chart version in the Helmfile configuration
around the version declaration, using a repository-native Helmfile render or
version-resolution check to verify the dependency resolves correctly. If such
validation is not applicable, document the justification in the PR instead.
- Line 42: Update the helm-nvcf-sis version in docs/version-catalog/main.yaml to
match the 1.18.7 value in the Helmfile, then regenerate docs/user/manifest.md
from the updated catalog so both documentation artifacts are synchronized.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a03f669-ef7b-41ca-9716-c46b62458bef
📒 Files selected for processing (1)
deploy/stacks/self-managed/helmfile.d/02-core.yaml.gotmpl
|
|
||
| - name: sis | ||
| version: 1.18.3 | ||
| version: 1.18.7 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add chart-version validation or explain why tests are not applicable.
This dependency change has no accompanying repository-native test or validation evidence. Add a Helmfile render/version-resolution check, or document in the PR why testing is not applicable.
As per coding guidelines: “Code changes must include tests, or the Pull Request must explain why tests are not applicable.”
🤖 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 `@deploy/stacks/self-managed/helmfile.d/02-core.yaml.gotmpl` at line 42, Add
validation for the chart version in the Helmfile configuration around the
version declaration, using a repository-native Helmfile render or
version-resolution check to verify the dependency resolves correctly. If such
validation is not applicable, document the justification in the PR instead.
Source: Coding guidelines
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find nearest AGENTS guidance files and inspect the relevant path scope.
printf '\n== AGENTS files ==\n'
find . -name AGENTS.md -print | sort
printf '\n== Relevant file snippets ==\n'
for f in \
deploy/stacks/self-managed/helmfile.d/02-core.yaml.gotmpl \
docs/user/manifest.md \
docs/version-catalog/main.yaml
do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
nl -ba "$f" | sed -n '1,260p' | rg -n 'helm-nvcf-sis|1\.18\.3|1\.18\.7|version:' -n -C 2 || true
else
printf '\n--- %s (missing) ---\n' "$f"
fi
done
printf '\n== Search for SIS version sources ==\n'
rg -n 'helm-nvcf-sis|1\.18\.3|1\.18\.7' deploy docs -g '!**/node_modules/**' || trueRepository: NVIDIA/nvcf
Length of output: 3124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Root and docs guidance ==\n'
for f in AGENTS.md docs/AGENTS.md; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
fi
done
printf '\n== References to version-catalog and manifest generation ==\n'
rg -n 'version-catalog|manifest\.md|helm-nvcf-sis|user/manifest|docs/version-catalog' . \
-g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' || true
printf '\n== Nearby context in docs/version-catalog/main.yaml ==\n'
sed -n '180,230p' docs/version-catalog/main.yaml
printf '\n== Nearby context in docs/user/manifest.md ==\n'
sed -n '78,100p' docs/user/manifest.mdRepository: NVIDIA/nvcf
Length of output: 36987
Sync the SIS version catalog and manifest. docs/version-catalog/main.yaml still pins helm-nvcf-sis to 1.18.3, so docs/user/manifest.md will stay out of sync with this Helmfile bump unless the catalog is updated and the manifest regenerated.
🤖 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 `@deploy/stacks/self-managed/helmfile.d/02-core.yaml.gotmpl` at line 42, Update
the helm-nvcf-sis version in docs/version-catalog/main.yaml to match the 1.18.7
value in the Helmfile, then regenerate docs/user/manifest.md from the updated
catalog so both documentation artifacts are synchronized.
2 participants
TL;DR
Update the self-managed stack SIS dependency from chart
1.18.3to1.18.7so deployments consume the release that enables NATS reconnect.Additional Details
SIS can retain a closed NATS connection after authentication expiry or a NATS restart. Subsequent JetStream publishes may not reach the worker plane, leaving function deployments in
DEPLOYING.Chart
1.18.7enables reconnect in the default SIS environment. This change updates only the downstream stack pin. It does not change release ordering, values overrides, or Kubernetes resource shapes.For the Reviewer
Review the SIS release entry in
deploy/stacks/self-managed/helmfile.d/02-core.yaml.gotmpl.For QA
git diff --check1.18.7contains the reconnect fix.Issues
Relates to #391
Checklist
Dependencies
helm-nvcf-sis1.18.7Summary by CodeRabbit