Add missing Langfuse env vars to operator deployment patches#377
Add missing Langfuse env vars to operator deployment patches#377sallyom merged 1 commit intoambient-code:mainfrom
Conversation
Add missing Langfuse observability environment variables to GitHub Actions operator deployment patches to match the base operator-deployment.yaml configuration. All env vars are configured with optional: true to prevent deployment failures if the ambient-admin-langfuse-secret doesn't exist. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: sallyom <somalley@redhat.com>
Claude Code ReviewSummaryThis PR adds missing Langfuse observability environment variables to GitHub Actions operator deployment patches, ensuring consistency with the base operator-deployment.yaml configuration. The changes are minimal, focused, and properly implement the Overall Assessment: ✅ LGTM - Ready to merge Issues by Severity🟡 Major Issues1. JSON Patch Maintenance Burden Location: All three workflow files (.github/workflows/components-build-deploy.yml:230, 284 and prod-release-deploy.yaml:255) Issue: The current approach uses inline JSON patches with the entire env array hardcoded in the workflow files. This creates significant maintenance issues:
Recommendation: Consider using a more maintainable approach: Option A - Kustomize Patches (Recommended): # .github/kustomize-patches/operator-env-patch.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
name: agentic-operator
spec:
template:
spec:
containers:
- name: agentic-operator
env:
- name: AMBIENT_CODE_RUNNER_IMAGE
value: "IMAGE_TAG_PLACEHOLDER"
# ... other env vars ...Then in workflows: # Single source of truth, just update image tags
sed -i "s|IMAGE_TAG_PLACEHOLDER|NEW_TAG|g" patch.yaml
kubectl patch deployment -n ambient-code --patch-file patch.yamlOption B - Template Script: ./scripts/update-operator-deployment.sh \
--runner-tag=NEW_RUNNER_TAG \
--backend-tag=NEW_BACKEND_TAGWhy this matters: The next time someone adds observability configs (e.g., OpenTelemetry, DataDog), or any new env var, they will need to remember to update 3 workflow files. This scales poorly. Reference: See CLAUDE.md Backend and Operator Development Standards - this type of duplication violates the DRY principle and makes the codebase harder to maintain. 🔵 Minor Issues1. Missing Workflow Testing Before Merge Issue: GitHub Actions workflows are only validated when they run. There is no pre-merge validation that the JSON patches are syntactically correct. Recommendation: Add a validation step in a pre-commit hook or CI check: # .github/workflows/validate-patches.yml
- name: Validate JSON patches
run: |
# Extract and validate each patch
grep -o "oc patch.*--type=json -p=" .github/workflows/*.y*ml | \
sed "s/.*-p=//" | sed "s/\x27$//" | \
jq . > /dev/null || exit 1Impact: Low priority - The current changes appear syntactically correct, but would prevent future typos. 2. No Documentation Update Location: Missing update to docs/observability-langfuse.md Issue: The observability documentation should mention that Langfuse env vars are automatically included in operator deployments via CI/CD workflows. Recommendation: Add a note to docs/observability-langfuse.md: ## CI/CD Integration
Langfuse environment variables are automatically configured in operator deployments:
- GitHub Actions workflows include LANGFUSE_* env vars in deployment patches
- All env vars use `optional: true` to allow deployments without Langfuse secret
- See .github/workflows/components-build-deploy.yml and prod-release-deploy.yamlImpact: Low - Documentation helps future maintainers understand the full deployment picture. Positive Highlights✅ Correct Use of ✅ Consistent with Base Configuration: The added env vars exactly match components/manifests/base/operator-deployment.yaml:62-85, ensuring deployment consistency across environments. ✅ Proper Secret Reference: Uses ✅ Comprehensive Coverage: Updates all three deployment workflows:
✅ Minimal Surface Area: Only touches the specific lines needed - no scope creep or unrelated changes. ✅ Security Best Practice: Langfuse credentials stored in Kubernetes secrets, not hardcoded in workflows. Follows the pattern established in operators/internal/handlers/sessions.go:303-323. RecommendationsHigh Priority (Future Work)
Low Priority
Verification Before Merge
Decision: Approve ✅ This PR correctly addresses the immediate issue (missing Langfuse env vars in CI/CD). The major issue identified (maintenance burden) is a pre-existing architectural concern, not introduced by this PR. The current fix is correct and should be merged. The refactoring recommendation is logged for future improvement but should not block this fix. |
Add missing Langfuse observability environment variables to GitHub Actions operator deployment patches to match the base operator-deployment.yaml configuration.
All env vars are configured with optional: true to prevent deployment failures if the ambient-admin-langfuse-secret doesn't exist.