feat!: migrate Helm chart from helm/v1-alpha to helm/v2-alpha by ian-flores · Pull Request #96 · posit-dev/team-operator

ian-flores · 2026-02-19T20:40:35Z

Summary

Migrate from deprecated kubebuilder helm/v1-alpha plugin to helm/v2-alpha, which generates charts dynamically from kustomize output
Upgrade kubebuilder from v4.5.1 to v4.12.0
Remove kube-rbac-proxy auth proxy sidecar from kustomize config (was not used in chart deployment)
Add hack/helm-post-generate.sh to apply customizations that v2-alpha overwrites on regeneration (namespace-scoped Role/RoleBinding, ServiceAccount annotations, replicas templating, imagePullSecrets, cert-manager volumes)

BREAKING CHANGE

Helm values structure changed — downstream consumers (PTD Pulumi) must update:

controllerManager.* → manager.*
controllerManager.container.image → manager.image
controllerManager.container.env (map) → manager.env (list)
certmanager.enable → certManager.enable

What's preserved

Chart.yaml metadata (version, description, maintainers)
README.md documentation
ServiceAccount annotation support (AWS IRSA)
Namespace-scoped RoleBinding for watched namespace
Metrics Service and cert-manager Certificate templates
CRD retention policy (helm.sh/resource-policy: keep)

What's new from v2-alpha

nameOverride / fullnameOverride support
NOTES.txt post-install instructions
Improved _helpers.tpl with safe name truncation (resourceName helper)
Metrics port parameterized via metrics.port
Makefile targets: helm-deploy, helm-status, helm-history, helm-rollback

Test plan

make helm-lint passes
make helm-template renders cleanly
make helm-generate is idempotent (zero diff on re-run)
Deploy to staging cluster with adhoc image and verify operator starts
Update PTD Pulumi code to use new values structure (separate PR)

Closes #85

Migrate from the deprecated kubebuilder helm/v1-alpha plugin to helm/v2-alpha, which generates charts dynamically from kustomize output. BREAKING CHANGE: Helm values structure changed: - controllerManager.* → manager.* - controllerManager.container.image → manager.image - controllerManager.container.env (map) → manager.env (list) - certmanager.enable → certManager.enable Closes #85

Address review findings from helm/v2-alpha migration: - Add namespace-scoped Role with all operator permissions to manager-role.yaml (was dropped by v2-alpha plugin) - Fix volumeMounts placement in manager.yaml (was incorrectly nested inside securityContext else branch) - Restore cert-manager Issuer and Certificate templates for webhook and metrics TLS - Restore metrics Service template for Prometheus scraping - Improve post-generate script robustness for volumeMounts insertion

claude · 2026-02-19T20:40:49Z

Claude encountered an error —— View job

Reviewing this PR...

Read review guidelines
Analyze PR diff and changed files
Review Helm chart changes (templates, values, helpers)
Review RBAC changes (security scrutiny)
Review hack/helm-post-generate.sh
Review Makefile changes
Submit review with inline comments

Update Pulumi Helm values to match the new team-operator chart structure after the helm/v1-alpha to helm/v2-alpha migration (posit-dev/team-operator#96). - controllerManager.* → manager.* - controllerManager.container.image → manager.image - controllerManager.container.env (map) → manager.env (list) - Remove digest-based image tag support (no longer needed)

claude · 2026-02-19T20:53:25Z

Claude encountered an error —— View job

Reviewing this PR...

Read review guidelines
Analyze PR diff and changed files
Review Helm chart changes (templates, values, helpers)
Review RBAC changes (security scrutiny)
Review hack/helm-post-generate.sh
Review Makefile changes
Submit review with inline comments

ian-flores · 2026-02-19T21:35:09Z

Local Code Review

Verdict: PASS with observations

Correctness

All core Kubernetes resources present and render correctly (helm lint + helm template pass)
Deployment, ServiceAccount, ClusterRole, Role, ClusterRoleBinding, RoleBinding, CRDs, Metrics Service, cert-manager Certificates all verified
make helm-generate is idempotent (zero diff on re-run)

RBAC Security

ClusterRole permissions identical to v1-alpha (PersistentVolumes only)
Namespace-scoped Role permissions identical to v1-alpha (all operator-managed resources)
No overly permissive wildcards, no cluster-admin escalation
Properly scoped to watched namespace

Important Observations

1. RBAC Helper Roles Removed (undocumented)
The old chart included 12 convenience ClusterRoles for end-user CRD management (e.g., site-editor-role, connect-viewer-role). These are removed and not regenerated by v2-alpha. rbacHelpers.enable exists in values.yaml but no corresponding templates exist. This should be documented as a removed feature.

2. Post-generate script heuristic
The Python block in hack/helm-post-generate.sh uses indentation-based heuristics for volumeMounts insertion. It has proper error handling (exits non-zero if insertion fails), but could benefit from a CI test that verifies cert-manager volumes render correctly:

helm template test dist/chart --set certManager.enable=true | \
  grep -q "mountPath: /tmp/k8s-webhook-server/serving-certs"

3. Migration guide gap
The chart README has no "Upgrading from v1-alpha" section documenting the values.yaml changes. Worth adding before merge.

Suggestions

Track upstream kubebuilder issues (#5486 replicas, #5489 env ergonomics) in the post-generate script comments
Add cert-manager volume rendering to CI verification step

…r CI test - Add "Upgrading from helm/v1-alpha" section to chart README with values mapping table and removed features documentation - Remove rbacHelpers config from values.yaml (no backing templates) - Add helm-test-certmanager Makefile target to verify cert-manager volumes render correctly - Add upstream kubebuilder issue tracking comments to post-generate script

…r CI test - Add "Upgrading from helm/v1-alpha" section to chart README with values mapping table and removed features documentation - Remove rbacHelpers config from values.yaml (no backing templates) - Add helm-test-certmanager Makefile target to verify cert-manager volumes render correctly - Add upstream kubebuilder issue tracking comments to post-generate script - Guard README migration guide insertion to prevent duplication

Merge main into helm-v2-alpha-migration branch. Resolved conflicts by keeping the v2-alpha manager.* values structure and discarding the old controllerManager.* structure from main. CRD files that were deleted by the v2-alpha migration (replaced by kubebuilder-generated equivalents) are kept deleted.

Changes: - Bump default memory limit from 128Mi to 256Mi in `dist/chart/values.yaml` to prevent OOMKills during heavy reconciliation - Update README.md to reflect the new memory limit default

Build succeeds. The test failures are pre-existing infrastructure issues (missing etcd binary for kubebuilder control plane tests) - unrelated to the README documentation changes. Changes: - Updated README parameter tables to use `manager.*` paths matching actual `values.yaml` structure (was stale `controllerManager.container.*`) - Updated security context and service account doc tables to use correct `manager.*` paths - Fixed inline installation example `--set` flags to use `manager.image.*` instead of `controllerManager.container.image.*` - Fixed all example values files (AWS, Azure, production, multi-namespace, node placement) to use `manager.*` structure with list-format env vars

The test failure is a pre-existing infrastructure issue (missing `etcd` binary for the test environment), not related to the README changes. Build passes cleanly. Changes: - Add "Manager Probes" documentation section noting that liveness/readiness probes are hardcoded with their current values - Add note to Azure Workload Identity example about the required `azure.workload.identity/use: "true"` pod label and that the chart doesn't currently support custom pod labels

All Connect-related tests pass. The `TestSiteReconcileWithExperimental` panic is a pre-existing infrastructure issue (missing kubebuilder etcd binary), not related to my changes. Changes: - Add test `TestSiteConnectTeardownIgnoredWhenEnabled` to verify the controller safely ignores `teardown=true` when `enabled` is true (or defaults to true), confirming the Connect CR remains active and unsuspended

Both subtests pass. Changes: - Convert `TestSiteConnectTeardownIgnoredWhenEnabled` to table-driven subtests covering both the default (nil) and explicit `Enabled: true` cases

All fixes verified. The Helm templates render correctly with: - Correct image: `posit/ptd-team-operator:v1.16.2` (appVersion fallback working) - ClusterRole now has proper labels Changes: - Fix image tag to default to `Chart.AppVersion` instead of `latest` (restores behavior from commit 0ee55c1) - Fix image repository from `posit/team-operator` to `posit/ptd-team-operator` to match CI publish target - Fix Makefile `get-helm-4` to `get-helm-3` (Helm 4 doesn't exist) - Add standard labels to ClusterRole in `manager-role.yaml` for consistency - Update `helm-post-generate.sh` to preserve these fixes across chart regeneration - Update README docs to reflect correct image repository and tag defaults

The v2-alpha chart uses manager.image.repository/tag instead of controllerManager.container.image.repository/tag.

The sed 's|||' command on line 430 used '|' as delimiter but the replacement string contained '|' (Go template pipe), causing "unknown option to 's'" on Linux sed. Switch to 'c\' (replace line) which avoids delimiter conflicts entirely.

ian-flores added 2 commits February 19, 2026 11:50

ian-flores mentioned this pull request Feb 19, 2026

fix: update team-operator Helm values for helm/v2-alpha chart posit-dev/ptd#132

Draft

3 tasks

ian-flores added 12 commits February 19, 2026 14:09

chore: regenerate helm chart CRDs

5885949

Address review findings (job 726)

3f726f3

Changes: - Bump default memory limit from 128Mi to 256Mi in `dist/chart/values.yaml` to prevent OOMKills during heavy reconciliation - Update README.md to reflect the new memory limit default

Address review findings (job 749)

309ff36

Both subtests pass. Changes: - Convert `TestSiteConnectTeardownIgnoredWhenEnabled` to table-driven subtests covering both the default (nil) and explicit `Enabled: true` cases

fix: update Kind test to use v2-alpha Helm value paths

b344f9e

The v2-alpha chart uses manager.image.repository/tag instead of controllerManager.container.image.repository/tag.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: migrate Helm chart from helm/v1-alpha to helm/v2-alpha#96

feat!: migrate Helm chart from helm/v1-alpha to helm/v2-alpha#96
ian-flores wants to merge 14 commits intomainfrom
helm-v2-alpha-migration

ian-flores commented Feb 19, 2026

Uh oh!

claude bot commented Feb 19, 2026 •

edited

Loading

Uh oh!

claude bot commented Feb 19, 2026 •

edited

Loading

Uh oh!

ian-flores commented Feb 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ian-flores commented Feb 19, 2026

Summary

BREAKING CHANGE

What's preserved

What's new from v2-alpha

Test plan

Uh oh!

claude bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ian-flores commented Feb 19, 2026

Local Code Review

Correctness

RBAC Security

Important Observations

Suggestions

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

claude bot commented Feb 19, 2026 •

edited

Loading

claude bot commented Feb 19, 2026 •

edited

Loading