Skip to content

Conversation

@Gkrumbach07
Copy link
Collaborator

No description provided.

@Gkrumbach07 Gkrumbach07 merged commit aa8801a into ambient-code:main Nov 6, 2025
7 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

PR Review: Update deployment path in production workflow for OpenShift components

Summary

This PR fixes the deployment path in the production release workflow by updating the kustomize apply command from components/manifests to components/manifests/overlays/production.


✅ Positive Aspects

  1. Correct Fix: The change properly targets the production overlay which contains production-specific configurations (OAuth patches, routes, namespace settings).

  2. Aligns with Kustomize Best Practices: Using overlays for environment-specific configurations is the standard pattern.

  3. Minimal Change: Single line change reduces risk and makes the fix easy to understand and review.

  4. Structural Correctness: The manifests directory structure follows proper kustomize patterns:

    • base/ contains common resources
    • overlays/production/ contains production-specific patches and resources
    • No kustomization.yaml at the root level (which would have been incorrect)

📋 Code Quality Assessment

Deployment Path (.github/workflows/prod-release-deploy.yaml:223):

  • ✅ Now correctly points to overlays/production which includes:
    • Production namespace configuration
    • OAuth proxy patches for frontend
    • OpenShift Routes
    • GitHub App secret
    • Production-specific image tags

Before: oc apply -k components/manifests ❌ (would fail - no kustomization.yaml at root)
After: oc apply -k components/manifests/overlays/production


🔍 Observations & Potential Improvements

1. Image Tag Inconsistency (Not introduced by this PR, but worth noting)

The production overlay still uses latest tags (overlays/production/kustomization.yaml:36-45), but the workflow patches deployments with versioned tags (:${{ needs.release.outputs.new_tag }}).

Current behavior:

  • Step 1: oc apply -k deploys with latest tags
  • Step 2-4: Patch commands update to versioned tags

Suggestion for future improvement:
Consider using kustomize edit set image before the apply command to eliminate the need for the three separate oc patch commands (lines 225-236).

2. Deployment Deletion Strategy (Line 218)

The workflow deletes all deployments before applying, which causes downtime:

oc delete deployment --all -n ambient-code

Consideration:

  • This ensures clean deployment but creates service interruption
  • The subsequent oc patch commands would trigger rolling updates without deletion
  • If this is intentional (to ensure clean state), consider adding a comment explaining the rationale

3. Missing Image Pull Policy Consistency

The operator patch sets IMAGE_PULL_POLICY=Always (line 236), but this is not consistently applied to frontend/backend patches.


🔒 Security Assessment

No security concerns introduced

  • Change is purely path correction
  • No secrets or credentials modified
  • No privilege escalation

🧪 Testing Considerations

What should be tested:

  • ✅ Deployment succeeds in production namespace
  • ✅ All resources from production overlay are applied (Routes, OAuth config, etc.)
  • ✅ Image tags are correctly updated by subsequent patch commands
  • ✅ Services remain accessible after deployment

Missing tests (not blocking, but good to have):

  • No automated test validates that the kustomize path is correct
  • Consider adding a dry-run validation step in future PRs

📊 Performance Impact

Neutral: No performance impact. The change corrects functionality rather than affecting performance.


✅ Verdict

LGTM - This is a correct and necessary fix.

Impact: Critical - Without this change, the deployment would fail since there is no kustomization.yaml at components/manifests/.

Recommendation: Approved for merge (already merged ✓)


📝 Follow-up Suggestions (Optional, Future Work)

  1. Add kustomization validation to CI/CD
  2. Consider consolidating image tag updates into single kustomize command
  3. Document the deployment deletion strategy (if intentional downtime is required)
  4. Add smoke tests after deployment to verify service availability

Reviewed by: Claude Code
Adherence to CLAUDE.md: ✅ Follows GitHub Actions CI/CD patterns documented in project guidelines

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.

1 participant