feat: add envVars field for Workbench, Connect, and Package Manager

[stevenolen]

Summary

Adds a new envVars field ([]corev1.EnvVar) to the Workbench, Connect, and Package Manager product specs (both the standalone product CRs and the Site-embedded specs). Unlike the existing addEnv (map[string]string, literals only), envVars supports the standard Kubernetes EnvVar structure including valueFrom (e.g. secretKeyRef), so secrets can be injected the Kubernetes-native way.

The existing addEnv field is deprecated in favor of envVars but continues to work. envVars is appended last when assembling the container Env, so it takes precedence on a name conflict.

Scope is intentionally limited to the three "product itself" pods. Workbench sessions already have sessionEnvVars; Connect content and Chronicle are not included.

Usage

apiVersion: core.posit.team/v1beta1
kind: Site
spec:
  packageManager:
    envVars:
      - name: PM_API_KEY
        valueFrom:
          secretKeyRef:
            name: my-secret
            key: api-key

The same envVars field is available under spec.connect and spec.workbench.

Changes

• API types: envVars added to Connect/Workbench/PackageManager specs and their Site-embedded internal specs; addEnv marked deprecated.
• Controllers: Site → product copy of envVars; envVars appended to the container Env render path for all three products.
• Regenerated: deepcopy, CRD bases (+ embedded copies), Helm chart CRDs, client-go applyconfiguration, openapi.
• Docs: docs/api-reference.md updated with envVars rows and addEnv deprecation notes.

Testing

• TestSiteReconciler_EnvVars — verifies Site → product CR propagation (including a secretKeyRef entry) for all three products.
• TestPackageManagerReconciler_EnvVars — verifies both a plain var and a secretKeyRef var reach the rendered Package Manager container Env.
• make manifests generate-all idempotent, make verify-crds, make fmt vet build, and make test all pass.

[stevenolen]

@claude review

[claude]

Claude finished @stevenolen's task in 2m 22s —— View job

---

Code Review: feat: add envVars field for Workbench, Connect, and Package Manager

• Read API type changes
• Read controller changes
• Read test changes
• Read docs changes
• Provide review feedback

---

Overall Assessment

This is a well-structured, cleanly scoped PR. The design choices are sound: exposing the full corev1.EnvVar type to enable valueFrom/secretKeyRef, deprecating but not removing addEnv, and placing envVars last in the ConcatLists chain to ensure precedence on name conflicts. The code is consistent across all three products and the Site→product propagation path.

Positives

1. Consistent placement — envVars is appended last in every product's Env slice (connect.go:687, package_manager.go:524, workbench.go:893), matching the documented precedence guarantee.
2. Good test coverage — TestSiteReconciler_EnvVars validates propagation for all three products including secretKeyRef; TestPackageManagerReconciler_EnvVars validates end-to-end rendering into the container spec.
3. Clean separation — The Internal*Spec types in site_types.go mirror the standalone types exactly, and the Site controllers do straightforward field copies.
4. Generated code in sync — deepcopy, CRD YAMLs, applyconfiguration, and Helm chart CRDs all appear correctly regenerated.

Minor Observations (non-blocking)

1. Deprecation enforcement — There's no validation webhook or admission rule that warns users still setting addEnv. This is fine for now since the field continues to work, but you may want a future TODO for surfacing a deprecation warning event or status condition.

2. Name-conflict semantics undocumented in code — The PR description says "envVars takes precedence on a name conflict," which is true because of ConcatLists ordering. However, Kubernetes itself uses the first occurrence of a given env var name in the container spec. So the current append-last-wins behavior relies on Kubernetes using the last entry, which is actually the opposite — Kubernetes uses the first entry. This means if the same var name appears in both addEnv and envVars, addEnv would actually win at runtime because it appears earlier in the slice.

   This contradicts the PR description's claim. You should either:

   • Reorder so envVars entries come before addEnv entries in the slice, or
   • Add deduplication logic that removes earlier entries with the same name when envVars provides a replacement, or
   • Update the documentation to clarify that envVars does not override addEnv on name conflict (users should remove the addEnv entry).

   Fix this →

3. No workbench controller-level test — There are tests for Site propagation and PackageManager rendering, but no analogous TestWorkbenchReconciler_EnvVars or TestConnectReconciler_EnvVars test that verifies the vars reach the rendered deployment container. The Site propagation test covers the "field appears on the CR" path, and the PM test covers the "CR → Deployment" path, which is decent, but adding one for Connect/Workbench would increase confidence given they have more complex Env slices.

4. []corev1.EnvVar{} placeholder in package_manager.go:523 — The empty slice literal between addEnv and envVars is harmless but unnecessary noise. Minor style nit.

Verdict

Looks good to merge with one item worth addressing: the env var precedence semantics (point #2 above). If the intent is truly for envVars to override addEnv, the ordering needs to be reversed since Kubernetes resolves duplicates using the first occurrence. If the intent is just "both work, remove the conflict yourself," then the docs should clarify.

---

[stevenolen]

Addressing the review points:

Point 2 (env var precedence) is incorrect — Kubernetes uses the last occurrence of a duplicate env var name, not the first. The kubelet builds the container environment from a name-keyed map with last-write-wins: tmpEnv[envVar.Name] = runtimeVal in makeEnvironmentVariables (kubelet_pods.go#L1426). Since envVars is appended after addEnv in ConcatLists, envVars takes precedence on a name conflict, which matches the PR description. No code change needed; the behavior is now documented on the deprecated addEnv field in 204381f.

Point 3 (missing render tests): addressed in 6183fa7 — added TestConnectReconciler_EnvVars and TestWorkbenchReconciler_EnvVars, each covering a literal value and a valueFrom.secretKeyRef reaching the rendered container env.

Point 1 (deprecation warning): intentionally deferred to keep this change minimal; addEnv still works. Reasonable follow-up.