-
Notifications
You must be signed in to change notification settings - Fork 13
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
ci: Add GH workflow to build app & deploy to GH Pages; Update demo link to point at GH Pages deployment. #114
Changes from 20 commits
718bc34
1ea5633
b395e12
5690e0d
b972057
e546847
e11f003
88bf319
3aea76c
6029019
df4a275
108ae1f
1bb76c5
10c351c
5545ca2
6b31dcd
a0ab914
dac2d1c
37911e7
ba82b3a
5c46893
8bccb9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,54 @@ | ||||||||||||||||||
name: "deploy-github-pages" | ||||||||||||||||||
|
||||||||||||||||||
on: | ||||||||||||||||||
push: | ||||||||||||||||||
branches: ["main"] | ||||||||||||||||||
|
||||||||||||||||||
permissions: | ||||||||||||||||||
# So the workflow can cancel in-progress jobs | ||||||||||||||||||
actions: "write" | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is my mistake in previous workflow PRs, but I found that this permission is not necessary for cancelling in-progress jobs. I think we should set these permissions to {} instead.
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
|
||||||||||||||||||
concurrency: | ||||||||||||||||||
group: "${{github.workflow}}-${{github.ref}}" | ||||||||||||||||||
# Cancel in-progress jobs for efficiency | ||||||||||||||||||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
cancel-in-progress: true | ||||||||||||||||||
|
||||||||||||||||||
jobs: | ||||||||||||||||||
build: | ||||||||||||||||||
runs-on: "ubuntu-latest" | ||||||||||||||||||
steps: | ||||||||||||||||||
- uses: "actions/checkout@v4" | ||||||||||||||||||
with: | ||||||||||||||||||
submodules: "recursive" | ||||||||||||||||||
|
||||||||||||||||||
- uses: "actions/setup-node@v4" | ||||||||||||||||||
with: | ||||||||||||||||||
node-version: "22" | ||||||||||||||||||
cache: "npm" | ||||||||||||||||||
Comment on lines
+23
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use LTS version of Node.js for production deployments. Node.js v22 is currently in beta. For production deployments, use an LTS version: - uses: "actions/setup-node@v4"
with:
- node-version: "22"
+ node-version: "20"
cache: "npm" 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usage of "22" is intended. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @junhaoliao, understood. I've noted that using Node.js version 22 is intended for this project. (꒦_꒦) ✏️ Learnings added
|
||||||||||||||||||
cache-dependency-path: "./package-lock.json" | ||||||||||||||||||
|
||||||||||||||||||
- run: "npm install" | ||||||||||||||||||
|
||||||||||||||||||
- run: "npm run build" | ||||||||||||||||||
|
||||||||||||||||||
- name: "Upload static files as artifact" | ||||||||||||||||||
uses: "actions/upload-pages-artifact@v3" | ||||||||||||||||||
with: | ||||||||||||||||||
path: "./dist" | ||||||||||||||||||
|
||||||||||||||||||
deploy: | ||||||||||||||||||
needs: "build" | ||||||||||||||||||
permissions: | ||||||||||||||||||
# To deploy to Pages | ||||||||||||||||||
pages: "write" | ||||||||||||||||||
|
||||||||||||||||||
# To request a JWT from GitHub for certifying the origin of the deployment | ||||||||||||||||||
id-token: "write" | ||||||||||||||||||
runs-on: "ubuntu-latest" | ||||||||||||||||||
environment: | ||||||||||||||||||
name: "github-pages" | ||||||||||||||||||
url: "${{steps.deployment.outputs.page_url}}" | ||||||||||||||||||
steps: | ||||||||||||||||||
- name: "Deploy to GitHub Pages" | ||||||||||||||||||
id: "deployment" | ||||||||||||||||||
uses: "actions/deploy-pages@v4" |
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.
🛠️ Refactor suggestion
Add workflow concurrency control to prevent deployment conflicts.
To prevent race conditions when multiple workflow runs are triggered, add concurrency control:
📝 Committable suggestion
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.
I pushed a fix in commit ba82b3a, please review it.
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.
@junhaoliao, thank you for adding the concurrency control to the workflow. This should effectively prevent deployment conflicts. The fix looks good.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!