-
Notifications
You must be signed in to change notification settings - Fork 31
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
edited staging workflow #2247
edited staging workflow #2247
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request enhance the GitHub Actions workflow for deploying multiple frontend applications when pull requests to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/deploy-frontends-to-staging.yml (3)
Line range hint
3-8
: Consider optimizing the workflow triggerThe current trigger configuration can be simplified by using
pull_request_target
instead ofpull_request
with explicittypes
field, as we only care about merged PRs.on: - pull_request: + pull_request_target: branches: - staging - types: - - closed
Line range hint
41-127
: Enhance change detection reliabilityThe current implementation has a few potential improvements:
- Using
HEAD^
might miss changes if the PR contains multiple commits- The workflow-trigger mechanism could be replaced with a more explicit approach
Consider these improvements:
- name: check modified frontends id: check_files run: | echo "=============== list modified files ===============" - git diff --name-only HEAD^ HEAD + git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.event.pull_request.head.sha }} echo "========== check paths of modified files ==========" - git diff --name-only HEAD^ HEAD > files.txt + git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.event.pull_request.head.sha }} > files.txtAlso, consider replacing the workflow-trigger file mechanism with GitHub Actions inputs or environment variables for more explicit control.
Line range hint
1-24
: Consider workflow optimization and security improvementsA few suggestions to enhance the workflow:
- Consider using composite actions to reduce duplication across jobs
- Add timeout-minutes to jobs to prevent hanging deployments
- Consider adding environment protection rules for staging
Example composite action structure:
# .github/actions/deploy-frontend/action.yml name: 'Deploy Frontend' inputs: app-name: required: true env-secret: required: true runs: using: "composite" steps: - name: Setup # Common setup steps...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/deploy-frontends-to-staging.yml
(1 hunks)website2/README.md
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- website2/README.md
🔇 Additional comments (2)
.github/workflows/deploy-frontends-to-staging.yml (2)
343-343
:
Fix potential line continuation issue
There appears to be a line continuation issue in the Docker build command for website2. The backslash at the end of line 343 seems to be on a new line, which could cause build failures.
- --build-arg NEXT_PUBLIC_OPENCAGE_API_KEY=${{ secrets.WEBSITE_NEXT_PUBLIC_OPENCAGE_API_KEY }} \
-
- --build-arg NEXT_PUBLIC_API_TOKEN=${{ secrets.WEBSITE_STAGE_NEXT_PUBLIC_API_TOKEN }} \
+ --build-arg NEXT_PUBLIC_OPENCAGE_API_KEY=${{ secrets.WEBSITE_NEXT_PUBLIC_OPENCAGE_API_KEY }} \
+ --build-arg NEXT_PUBLIC_API_TOKEN=${{ secrets.WEBSITE_STAGE_NEXT_PUBLIC_API_TOKEN }} \
Likely invalid or redundant comment.
Line range hint 1-607
: Verify deployment order dependencies
While the workflow functions correctly, consider adding explicit job dependencies to ensure proper deployment order when multiple applications are updated simultaneously.
✅ Verification successful
No explicit deployment order dependencies found, but some services share API integrations
The analysis reveals that while there are API integrations between services (like analytics being consumed by the platform and website), the current parallel deployment strategy is appropriate because:
- Services maintain API version compatibility
- Each service deployment is independent and includes its own configuration
- Cross-service communication happens through stable API endpoints
The existing workflow correctly handles this by:
- Using separate jobs for each service
- Maintaining independent configuration management
- Deploying services in parallel when changes affect multiple services
Note: The workflow-trigger
file already implements a safety mechanism by triggering all deployments when global changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential deployment order dependencies in the codebase
# Look for import statements or API calls between applications
rg -g '*.{js,jsx,ts,tsx}' -B 2 -A 2 'import.*from.*@airqo' || true
rg -g '*.{js,jsx,ts,tsx}' -B 2 -A 2 'fetch\(.*airqo' || true
Length of output: 129
Script:
#!/bin/bash
# Let's check for API dependencies between applications by looking for URLs and endpoints
# Search for API calls or URLs containing service names
echo "=== Checking for cross-service API calls ==="
rg -g '*.{js,jsx,ts,tsx,json}' -i '(netmanager|platform|calibrate|analytics|inventory|docs|reports).*api' || true
# Check for environment variables referencing other services
echo "=== Checking environment files for service dependencies ==="
fd -e env -e env.example -e env.sample -X cat {} 2>/dev/null || true
# Check package.json files for dependencies between apps
echo "=== Checking package.json files for dependencies ==="
fd package.json -x cat {} | grep -i "airqo" || true
# Check docker-compose files for service dependencies
echo "=== Checking Docker/K8s files for service dependencies ==="
fd -e yaml -e yml -g '*docker-compose*' -g '*values*.y*ml' -X cat {} 2>/dev/null || true
Length of output: 5891
New Website2 changes available for preview here |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
New Features
Bug Fixes
website2
README file by removing an extraneous period in the "Getting Started" section.