-
Notifications
You must be signed in to change notification settings - Fork 229
ci: deploy vrts on main branch correctly #5569
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
base: main
Are you sure you want to change the base?
Conversation
|
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
@TarunAdobe How can i verify this? can you create a section on what was the issue and what is the fix for the context? |
Yusss!! will do |
.circleci/config.yml
Outdated
# Get PR number from CircleCI environment | ||
PR_NUMBER="" | ||
if [ -n "$CIRCLE_PULL_REQUEST" ]; then | ||
PR_NUMBER=$(echo $CIRCLE_PULL_REQUEST | sed 's/.*\/pull\///') |
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_NUMBER=$(echo $CIRCLE_PULL_REQUEST | sed 's/.*\/pull\///') | |
PR_NUMBER=$(echo "$CIRCLE_PULL_REQUEST" | sed 's/.*\/pull\///') |
Unquoted variables can break if they contain spaces or special characters
Added a test commit to verify that checking for branch name works. |
Maybe a bit outside the initial scope of this task, but I just noticed https://swcpreviews.z13.web.core.windows.net/beta/docs/ does not load correctly. Is it something we could take a look at in this PR? |
a99e97e
to
11c4916
Compare
.circleci/config.yml
Outdated
if [ -n "$PR_NUMBER" ]; then | ||
echo "Deploying VRT for PR #$PR_NUMBER (via GitHub API)" | ||
# Set deployment path based on branch | ||
if [ "$CIRCLE_BRANCH" = "ttomar/vrts-main" ]; then |
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.
Forgive my intruding if this is still in testing but is this meant to be committed?
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.
not meant to be committed at all. would remove when somebody will approve the pr
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.
nit: As @rubencarvalho suggested if you want to have the beta/docs rectified here its nice to have or else let's create a follow up PR and add an effort. Looks good. Thanks for doing it.
already done heehhe |
60d49f2
to
34210b0
Compare
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.
🚢
Can you share what the skipped check is doing? or why its skipped? |
skipped check is to clean up PR deployments from azure blob storage. it only runs once the pr is closed. |
Description
Updated CircleCI configuration to ensure Visual Regression Test (VRT) results are properly deployed to Azure blob storage when running on the main branch, not just on pull requests.
Motivation and context
Previously, VRT results were only being deployed for pull requests, but main branch VRT runs were not being deployed to the preview site. This made it difficult to access and review VRT results from main branch builds, which are important for baseline comparisons and debugging.
Related issue(s)
Screenshots (if appropriate)
N/A
Author's checklist
Reviewer's checklist
patch
,minor
, ormajor
featuresManual review test cases
VRT deployment should work on main branch
https://swcpreviews.z13.web.core.windows.net/main/express-dark-large-rtl/review/
Device review