fix(harbor): preserve jobservice and trivy storageClass on upgrade - #2930

Code Review

This pull request addresses issue #2368 in the Harbor package by preserving the existing storageClassName of the jobservice PVC during upgrades via a cluster lookup, falling back to the configured storageClass on fresh installs. It also adds a test target to the Makefile and introduces a comprehensive helm-unittest suite to verify this behavior. The review feedback suggests using the dig function when accessing nested fields on the looked-up PVC to prevent potential template rendering errors if the spec field is missing or nil.

Actionable comments posted: 1

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 `@packages/apps/harbor/templates/harbor.yaml`:
- Around line 34-38: The issue is that the jobservice storage class lookup uses
`| default ""` which treats an explicitly set empty string (`""` in
`spec.storageClassName`) as falsy and collapses it, losing the explicit "no
dynamic provisioning" state. Fix this by using a conditional that checks whether
the `spec.storageClassName` field exists (not just whether it is truthy) so that
explicitly empty values are preserved as `"-"`. Apply this fix to both the
initial jobservice PVC lookup block around line 37 (where
`$jobserviceStorageClass` is assigned from
`$jobservicePvc.spec.storageClassName`) and to any other similar storage class
assignment blocks that reference the jobservice PVC (around lines 125-131),
ensuring that explicit empty strings are maintained across upgrades instead of
being omitted from the template output.

LGTM — approving. No blockers: the jobservice PVC storageClass is correctly threaded, and the lookup-preserve logic keeps the rendered manifest equal to the live object so the immutable-field rejection on upgrade is avoided.

Business context: Harbor's jobservice PVC was rendered without a storageClassName, so default-StorageClass admission stamped a class onto the live PVC that the rendered manifest never carried; the resulting drift against the immutable PVC field broke upgrades of the harbor system HelmRelease.

I verified the fix against the upstream subchart and the live-PVC semantics. The problem is real — upstream emits storageClassName only when jobLog.storageClass is set, and main never passed it through for jobservice. The common path is correct: an existing PVC's class is read back and re-rendered so the manifest matches the live object; a fresh install falls back to .Values.storageClass, and a fresh install with an empty storageClass self-heals on the next reconcile once lookup observes the admission-defaulted class.

Blockers: none.

Non-blocking:

1. Explicit-empty PVC class is not preserved. When an existing jobservice PVC has spec.storageClassName: "" (upstream's - / no-dynamic-provisioning state), lookup ... | default "" collapses to falsy and the {{- with $jobserviceStorageClass }} block is skipped, so the rendered manifest omits storageClassName instead of reproducing "". That deviates from the PR's own "keep rendered equal to live" invariant. Narrow in practice — a cluster with a default StorageClass never produces "" — but it is the one input the preserve logic does not round-trip. Evidence: packages/apps/harbor/templates/harbor.yaml:37 (| default "") and :127 ({{- with ... }}), against upstream packages/system/harbor/charts/harbor/templates/jobservice/jobservice-pvc.yaml:25-31 (- renders storageClassName: "").

2. Fixes #2368 over-closes the issue. The linked issue covers both the jobservice PVC and the trivy StatefulSet VCT; this PR addresses only jobservice, while the trivy block still passes .Values.storageClass blindly into an immutable VCT. On merge the issue auto-closes with the trivy half unfixed. Consider Refs #2368 (or a dedicated trivy follow-up) so the trivy immutability is not silently dropped. Evidence: issue #2368 body lists both PVCs; packages/apps/harbor/templates/harbor.yaml:132-138 (trivy still uses .Values.storageClass, unchanged from main).

3. The upgrade path itself is not unit-tested. helm-unittest renders lookup as empty, so all four new cases exercise only the fresh-install fallback; the preserve-existing-class branch — the actual upgrade-safety logic — is covered only by the manual server-side dry-run noted in the description. This is an inherent helm-unittest limitation, not a request to add a test. Evidence: packages/apps/harbor/tests/jobservice_storageclass_test.yaml (every case sets or omits storageClass; none mocks an existing PVC).

LGTM — propagates storageClass to the jobservice PVC and trivy VCT with a lookup-preserve guard, fixing the immutable-storageClassName drift from #2368.

Verified:

• PVC naming is correct: harbor.fullnameOverride: {{ .Release.Name }} makes the upstream subchart name resources <release>-*, so <release>-jobservice and data-<release>-trivy-0 are the right lookup keys.
• The lookup-preserve logic is sound across all four states: fresh install (configured class), existing PVC (preserve live class), explicit "" (round-trips via the "-" sentinel), and no storageClassName key (stays omitted to match live).
• | quote is necessary, not cosmetic: an unquoted - sentinel would be ambiguous YAML; quoting yields "-".
• lookup is an established, working pattern in this very chart (credentials/token-cert preservation already rely on it), which de-risks the Flux helm-controller concern.

Non-blocking:

• PR/commit body says it "wires a test: target so make unit-tests stops skipping packages/apps/harbor", but the test: target and a tests/ suite already exist on main — harbor was already covered. The new suite does run; only the description is inaccurate. Worth correcting so git log stays accurate.
• helm-unittest can't exercise the preserve branches (lookup returns nil there), so only the fresh-install fallback is covered in CI; the actual new behavior rests on the manual server-side dry-run. An e2e assertion would close the loop.
• db/redis still forward .Values.storageClass without the lookup-preserve guard; if those PVCs can hit the same #2368 drift when storageClass is unset, they'd need the same treatment — confirm they're immune (operator-managed) or out of scope.