Skip to content

Add missing Langfuse env vars to operator deployment patches#377

Merged
sallyom merged 1 commit intoambient-code:mainfrom
sallyom:add-operator-env
Nov 26, 2025
Merged

Add missing Langfuse env vars to operator deployment patches#377
sallyom merged 1 commit intoambient-code:mainfrom
sallyom:add-operator-env

Conversation

@sallyom
Copy link
Collaborator

@sallyom sallyom commented Nov 26, 2025

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.

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>
@github-actions
Copy link
Contributor

Claude Code Review

Summary

This 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 optional: true flag to prevent deployment failures when the secret does not exist.

Overall Assessment: ✅ LGTM - Ready to merge

Issues by Severity

🟡 Major Issues

1. 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:

  • Duplication: The same env array is duplicated across 3 workflow files (2 locations in components-build-deploy.yml, 1 in prod-release-deploy.yaml)
  • Sync Risk: When new env vars are added (like Langfuse), all 3 locations must be manually updated. This PR would not have been necessary if using a better pattern
  • Readability: 14 environment variables in a single-line JSON string is hard to review and error-prone

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.yaml

Option B - Template Script:
Create a helper script that reads from base operator-deployment.yaml and updates only image tags:

./scripts/update-operator-deployment.sh \
  --runner-tag=NEW_RUNNER_TAG \
  --backend-tag=NEW_BACKEND_TAG

Why 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 Issues

1. 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 1

Impact: 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.yaml

Impact: Low - Documentation helps future maintainers understand the full deployment picture.


Positive Highlights

Correct Use of optional: true: All four Langfuse env vars properly use optional: true, matching the base operator-deployment.yaml pattern. This prevents pod startup failures when ambient-admin-langfuse-secret does not exist.

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 secretKeyRef pointing to ambient-admin-langfuse-secret, consistent with the Langfuse integration in PR #329.

Comprehensive Coverage: Updates all three deployment workflows:

  • components-build-deploy.yml (main branch deploys)
  • components-build-deploy.yml (dispatch deploys)
  • prod-release-deploy.yaml (release deploys)

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.


Recommendations

High Priority (Future Work)

  1. Refactor workflow env patches to use Kustomize or templating to eliminate duplication (see Major Issue Outcome: Reduce Refinement Time with agent System #1)
  2. Consider adding a CI validation step for JSON patch syntax

Low Priority

  1. Update docs/observability-langfuse.md to document CI/CD integration
  2. Add a comment in workflow files explaining the env var source: # Env vars must match components/manifests/base/operator-deployment.yaml

Verification Before Merge

  • Verify CI checks pass (component builds, linting, e2e tests)
  • Confirm deployment works in staging environment
  • Verify operator pods restart successfully after patch application

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.

@sallyom sallyom merged commit 3b30d05 into ambient-code:main Nov 26, 2025
20 checks passed
@bobbravo2 bobbravo2 added this to the v0.0.11 milestone Jan 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants