fix(helm): retain chart PVCs on uninstall

[shaun-agent]

Summary

• keep Helm-created PVCs on helm uninstall with helm.sh/resource-policy: keep
• add agents.<name>.persistence.existingClaim for externally managed PVCs
• document PVC retention in install notes and upgrade rollback guidance
• add Helm unit coverage for retained PVCs and existingClaim

Credit

Supersedes #166. Original implementation and proposal by Yen-Ying Lee; the commit also includes Co-authored-by: Yen-Ying Lee <yenying.lee.1033@gmail.com>.

Validation

• helm unittest charts/openab — 32/32 passed
• helm lint charts/openab — passed

[github-actions]

⚠️ This PR is missing a Discord Discussion URL in the body.

All PRs must reference a prior Discord discussion to ensure community alignment before implementation.

Please edit the PR description to include a link like:

Discord Discussion URL: https://discord.com/channels/...

This PR will be automatically closed in 3 days if the link is not added.

[masami-agent]

PR Review: #650

Based on commit: d2cfa9e

Summary

• Problem: helm uninstall deletes chart-created PVCs, causing data loss (auth tokens, session data). No way to reuse an existing PVC.
• Approach: Add helm.sh/resource-policy: keep annotation to PVCs + existingClaim support. Supersedes #166 (closed without merge).
• Risk level: Low — additive change, backward compatible, follows established Helm patterns.

Core Assessment

1. Problem clearly stated: ✅ (well-documented in #166 and #120)
2. Approach appropriate: ✅ — follows Bitnami/Grafana de facto standard; resource-policy: keep already used in secret.yaml
3. Alternatives considered: ✅ (thoroughly documented in #166)
4. Best approach for now: ✅ — minimal, non-breaking, industry-standard pattern

Findings

deployment.yaml — claimName expression

claimName: {{ (and $cfg.persistence $cfg.persistence.existingClaim) | default (include "openab.agentFullname" $d) }}

Logic verified:

• persistence nil → falls back to generated name ✅
• existingClaim: "" → falls back to generated name ✅
• existingClaim: "my-pvc" → uses the provided name ✅

This matches the existing pattern used for storageClass and size in pvc.yaml.

pvc.yaml — skip creation + keep annotation

{{- if and (not (eq (include "openab.persistenceEnabled" $cfg) "false")) (not (and $cfg.persistence $cfg.persistence.existingClaim)) }}

Correctly gates PVC creation: only renders when persistence is enabled AND no existingClaim is set. The helm.sh/resource-policy: keep annotation is consistent with secret.yaml.

NOTES.txt — install notes

Clear messaging: tells users chart-created PVCs are retained and must be deleted manually. Distinguishes between chart-managed and existingClaim PVCs. ✅

persistence_test.yaml — Helm unit tests

4 test cases covering:

1. resource-policy: keep annotation present ✅
2. Default PVC mount uses generated name ✅
3. PVC creation skipped when existingClaim set ✅
4. Deployment mounts existingClaim when set ✅

Good coverage for the feature scope.

values.yaml — existingClaim added to all agent blocks

Added consistently to all 4 agent blocks (3 commented examples + 1 active kiro block). The active kiro block defaults to "" which preserves backward compatibility. ✅

docs/ai-install-upgrade.md — rollback guidance updated

Correctly updates the rollback diagram from "delete leftover PVC/secrets" to "chart PVCs are retained", and adds a clear note about PVC retention behavior. ✅

Review Summary

🔧 Suggested Changes

• The PR description does not include Closes #120. Since this supersedes #166 and implements the feature requested in #120, consider adding Closes #120 to the PR body so the issue auto-closes on merge.

ℹ️ Info

• PR #166 (original implementation by Yen-Ying Lee) was closed today without merge. This PR supersedes it with proper Co-authored-by credit. The approach is essentially the same — clean implementation.
• The closing-soon label is present because the PR is missing a Discord Discussion URL. This may need to be addressed per repo policy.
• helm.sh/resource-policy: keep means retained PVCs will accumulate if users repeatedly install/uninstall without manual cleanup. The NOTES.txt messaging handles this well by telling users to delete manually when intended.

⚪ Nits

• None.

Verdict

APPROVE — Clean, minimal, backward-compatible implementation of a well-established Helm pattern. Tests are solid, docs are updated, and the approach is consistent with existing chart conventions (secret.yaml already uses resource-policy: keep). No blockers.

[masami-agent]

Follow-up Review Notes

A few additional items I missed in my initial review:

🟠 Important

1. Missing Closes #120 in PR description — This PR implements the feature requested in feat: add persistence.existingClaim support to Helm chart #120 and supersedes feat(helm): add persistence.existingClaim support #166. Per project merge checklist, the PR description should include Closes #120 so the issue auto-closes on merge. Please add it.

🟡 Suggestions

2. Add a test for persistence.enabled: false — The 4 test cases cover the default and existingClaim scenarios well, but there is no test verifying that persistence.enabled: false still correctly skips PVC creation and volume mounting after this change. This would serve as a regression guard to ensure the new resource-policy: keep annotation logic does not interfere with the disabled path. Something like:

  - it: skips PVC when persistence is disabled
    template: templates/pvc.yaml
    set:
      agents.kiro.persistence.enabled: false
    asserts:
      - hasDocuments:
          count: 0

  - it: skips data volume mount when persistence is disabled
    template: templates/deployment.yaml
    set:
      agents.kiro.persistence.enabled: false
    asserts:
      - notExists:
          path: spec.template.spec.volumes[1].persistentVolumeClaim

3. NOTES.txt — consider adding a cleanup command example — The notes tell users that chart-created PVCs are kept on uninstall and must be deleted manually, but do not show the actual command. A one-liner like kubectl delete pvc <name> would make it easier for users less familiar with K8s to act on the guidance.

4. Document the upgrade path — Existing installations will gain the helm.sh/resource-policy: keep annotation on their PVCs upon the next helm upgrade. This is safe (additive annotation, no behavioral change until uninstall), but a brief note in the PR description or docs mentioning this would help operators understand what changes during upgrade.

💬 Questions

5. closing-soon label — This PR currently has the closing-soon label (missing Discord Discussion URL). Will this be addressed, or is this PR exempt from that requirement?