-
Notifications
You must be signed in to change notification settings - Fork 566
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
Add landing page and upload vrt/aat reports to GitHub Pages #5499
Conversation
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
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.
This makes me so happy I could cry 😂 the days of downloading VRT reports are over!
I would wait to merge until someone from eng reviews, but from a user perspective I APPROVE
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.
Thanks so much for doing this! 🥳 Just left a question to get your take on it, curious to hear what you say 👀
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.
Just thinking out loud, would it be possible for the required jobs to stay in the ci workflow and then have deploy_preview
's trigger be workflow_run
? Then we could use that deploy_preview
workflow to download artifacts from the CI
workflow that we would like to host/deploy.
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 can give that a try. I think I was initially trying to avoid deploy_preview
from waiting for ci
and parallelize the reports running to run but it probably won't have a significant impact of time. Also it would minimize the changes being made at this point.
Also, super random, but do you happen to know what permissions we need to clean up old environments/deployments? 👀 I remember trying to look into this in the past but couldn't figure it out and was curious if you knew. This is starting to come up more once we started having two deployments per-PR and since we're not removing any of them our list of environments is getting very long 😅 |
I looked at it briefly and came across https://github.com/strumwolf/delete-deployment-environment. I was thinking we could potentially run a workflow when I PR is closed/merged to delete the envs for that PR. |
@joshblack It doesnt look like |
@hussam-i-am oh you're totally right, the event right now is
|
Yeah I updated deploy_preview to use workflw_run instead of pull_request but movie thhings back into ci.yml. ci runs but deploy_preview is not being run when it completes. |
Ohhh, sorry @hussam-i-am! Totally misunderstood you, my bad 👍 I think |
- name: Download VRT reports (All flags enabled) | ||
uses: actions/download-artifact@v4 | ||
with: | ||
name: vrt-all-flags | ||
path: docs/public/vrt-all-flags | ||
- name: Download VRT reports (No flags enabled) | ||
uses: actions/download-artifact@v4 | ||
with: | ||
name: vrt-no-flag | ||
path: docs/public/vrt-no-flag | ||
- name: Download AAT reports (All flags enabled) | ||
uses: actions/download-artifact@v4 | ||
with: | ||
name: axe-all-flags | ||
path: docs/public/aat-all-flags | ||
- name: Download AAT reports (No flags enabled) | ||
uses: actions/download-artifact@v4 | ||
with: | ||
name: axe | ||
path: docs/public/aat-no-flag |
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.
For actions/download-artifact
, would we need to pass along run-id
to these in order to download from the CI workflow? My assumption is that these try to download from the current workflow but let me know if that's incorrect
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.
That was also my assumption and they seem to be updating correctly on each run
* Revert "Fix Prod Storybook deployment (#5535)" This reverts commit 8b96591. * Revert "fix: deploy_preview workflow (#5529)" This reverts commit be42cc6. * Revert "Fixing deploy_preview workflow (#5526)" This reverts commit 62353f2. * Revert "Remove branch from preview_deploy.yml (#5523)" This reverts commit e2075bd. * Revert "Add landing page and upload vrt/aat reports to GitHub Pages (#5499)" This reverts commit 5a6d7bb.
Changelog
New
Changed
deploy_preview.yml
workflow #5523Removed
Rollout strategy
Workflow updates, nothing to roll out
Testing & Reviewing
Merge checklist