-
Notifications
You must be signed in to change notification settings - Fork 8
Update deploy workflow to have consolidated Frontend Deployment flow #960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…tep to set deployment variables
|
✅ Deploy Preview for learncarddocs canceled.
|
❌ Deploy Preview for staging-learncardapp failed. Why did it fail? →
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
The PR consolidates two frontend deployment workflows into one, using dynamic variables based on target environment. The refactoring generally looks solid, but there's a critical behavior change in the npm publishing dependency and some maintainability concerns with complex conditional logic.
2 issues detected:
🐞 Bug - Adding deploy-frontend to the needs array creates an unnecessary blocking dependency that wasn't present before. 🛠️
Details: The npm publishing job now depends on frontend deployment completion, which changes the workflow behavior. Previously, npm packages could publish independently of frontend deployments. This could significantly delay npm releases if frontend deployment is slow or fails.
File:.github/workflows/deploy.yml (555-555)
🛠️ A suggested code correction is included in the review comments.🧹 Maintainability - Nested ternary operators with multiple conditions reduce code readability and maintainability. 🛠️
Details: The environment selection uses deeply nested ternary operators that are difficult to read and maintain. With multiple conditions checking event types and input values, the logic is hard to follow and error-prone for future modifications.
File:.github/workflows/deploy.yml (373-373)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
|
This PR is missing a Jira ticket reference in the title or description. |
|
🥷 Code experts: TaylorBeeston TaylorBeeston has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
LGTM
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
Overview
🎟 Relevant Jira Issues
📚 What is the context and goal of this PR?
Previously, the deploy.yml GitHub Actions workflow had two separate jobs for deploying frontend applications:
deploy-lca-frontend- For the LearnCard Appdeploy-scouts-frontend- For the Scouts AppBoth jobs contained nearly identical logic with only minor differences in configuration values (Netlify branch, CapGo app ID, build paths, etc.). This led to ~160 lines of duplicated YAML code and required maintaining two separate workflow inputs in the Actions UI.
🥴 TL; RL:
💡 Feature Breakdown (screenshots & videos encouraged!)
1. Consolidated Workflow Inputs
deploy-lca-frontendanddeploy-scouts-frontendcheckboxesdeploy-frontendcheckbox labeled "Deploy Frontend (Netlify + CapGo)"2. Merged Deployment Jobs
Replaced both frontend jobs with a single
deploy-frontendjob that dynamically sets configuration based on the selectedtarget-environment.3. Resolved Workflow Structure
determine-affectedtarget_frontend_env,target_brain_service_env, etc.) so each job just references${{ needs.determine-affected.outputs.target_*_env }}deploy-frontendSet Deployment Variablesstep to switch between LCA and Scouts configs based ontarget-environmentinputpush-docker-imagespublish-npmdeploy-frontend(ensuring packages publish only after full deploy)"Set Deployment Variables" step outputs the correct values based on environment:
app_idscoutslcaproject_namescoutslearn-card-appnetlify_branchproduction-scoutsproductioncapgo_idorg.scoutpass.appcom.learncard.appbuild_pathapps/scouts/buildapps/learn-card-app/buildforce_pushtruefalse4. Updated Job Dependencies
The
publish-npmjob now depends on the consolidateddeploy-frontendjob.🛠 Important tradeoffs made:
🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
I'd have to merge in main to test properly
📱 🖥 Which devices would you like help testing on?
🧪 Code Coverage
Documentation
📝 Documentation Checklist
User-Facing Docs (
docs/→ docs.learncard.com)docs/tutorials/)docs/how-to-guides/)docs/sdks/)docs/core-concepts/)docs/apps/)Internal/AI Docs
Visual Documentation
💭 Documentation Notes
✅ PR Checklist
🚀 Ready to squash-and-merge?:
✨ PR Description
Purpose: Refactor GitHub Actions deployment workflow to consolidate frontend deployments using dynamic environment mapping and centralized configuration.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how