-
Notifications
You must be signed in to change notification settings - Fork 227
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 |
# 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
elif [ -n "$CIRCLE_PR_NUMBER" ]; then | ||
PR_NUMBER="$CIRCLE_PR_NUMBER" | ||
echo "Deploying VRT for PR #$PR_NUMBER" | ||
elif [[ "$CIRCLE_BRANCH" =~ ^pull/[0-9]+$ ]]; 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.
This line assumes a format that may not always hold. CIRCLE_BRANCH
for forked PRs is typically the branch name of the fork, not pull/xxx
. You can consider removing if this condition even triggers.
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.
we dont run circleci on forks so idt we need to worry abt this
elif [ -n "$CIRCLE_PR_NUMBER" ]; then | ||
PR_NUMBER="$CIRCLE_PR_NUMBER" | ||
echo "Deploying HCM VRT for PR #$PR_NUMBER" | ||
elif [[ "$CIRCLE_BRANCH" =~ ^pull/[0-9]+$ ]]; 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.
same as above
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.
we dont run circleci on forks so idt we need to worry abt this
# GitHub API fallback | ||
PR_NUMBER=$(curl -s -H "Authorization: Bearer $GITHUB_TOKEN" \ | ||
-H "Accept: application/vnd.github+json" \ | ||
"https://api.github.com/repos/$CIRCLE_PROJECT_USERNAME/$CIRCLE_PROJECT_REPONAME/pulls?head=$CIRCLE_PROJECT_USERNAME:$CIRCLE_BRANCH&state=open" \ |
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.
GitHub allows multiple PRs from different forks to use the same branch name.This can cause the query to match a different PR from a different user, leading to incorrect PR numbers being picked.
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.
we don't run circleci on forked branches
Added a test commit to verify that checking for branch name works. |
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