refactor(helm): externalize chart credentials via SecretRef only (FLPATH-4806) - #66
Conversation
PR Summary by QodoRequire external SecretRefs for Helm chart credentials
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
1.
|
| # External kubeconfig mode (pre-existing Secret): | ||
| kubectl create secret generic my-kubeconfig-secret \ | ||
| --from-file=kubeconfig=/path/to/kubeconfig | ||
| helm upgrade dcm deploy/helm/dcm --reuse-values \ |
There was a problem hiding this comment.
Are old keys (postgres.user, auth.proxySecret, inline kubeconfig/pullSecret) exists helm upgrade --reuse-values on an existing install will fail or attach the wrong Secret.
There was a problem hiding this comment.
We haven't had a real release yet, so there shouldn't be existing installs that need an upgrade migration. Given that, are you okay with us proceeding as-is, or do you have another approach you'd prefer?
LinskId
left a comment
There was a problem hiding this comment.
after resolving comments
…ATH-4806) Remove inline DB and auth credentials from values/schema and delete chart-managed secret templates. Require pre-created Kubernetes Secrets, update Keycloak realm import for env placeholders, and align verify scripts. Compose realm source intentionally unchanged; pairs with compose split PR. https: //redhat.atlassian.net/browse/FLPATH-4806 Signed-off-by: Chad Crum <ccrum@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
Signed-off-by: Chad Crum <ccrum@redhat.com>
89dd2b1 to
f870d32
Compare
jordigilh
left a comment
There was a problem hiding this comment.
Suggestion
The current checks are green. One compatibility consideration is worth documenting before merge: this change removes several chart values and Secret templates, which could affect helm upgrade --reuse-values if existing deployments use those keys. I am not assuming that such deployments exist.
If this chart is intentionally greenfield-only, please state that explicitly. If existing deployments are supported, consider adding a migration note or an upgrade test covering the removed values. This is a suggestion, not a confirmed blocker.
Current review position: No blocking finding from this suggestion.
Summary
${AUTH_PROXY_SECRET}and${DCM_DEV_USER_PASSWORD}placeholders.Related PRs
Expected CI failure
The Helm chart job is expected to fail on this PR until the companion changes land. PR #60 updates
deploy/keycloak/realm-export.json(the Compose source) but intentionally leavesdeploy/helm/dcm/files/realm-export.jsonunchanged; this PR updates the Helm copy. While either PR is tested alone,helm-chart-verify-syncreports a stale or mismatched realm file. Once PRs #60 and #46 are merged, the Helm CI test is expected to pass.Issue
https://redhat.atlassian.net/browse/FLPATH-4806