-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat(e2e): generate branch with the updated e2e screenshots #2915
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit e5f7d4c.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files☔ View full report in Codecov by Sentry. |
id: vars | ||
env: | ||
# run_id and run_attempt are needed to avoid conflict in the branch name | ||
BRANCH_NAME: ${{ github.head_ref }}${{ inputs.branchSuffix }}-${{ github.run_id }}-${{ github.run_attempt }} |
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.
shouldn't we update the previous branch when re-running a workflow?
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.
The action (stefanzweifel/git-auto-commit-action) does not support that from what I understood.
because we ask to create the branch so if the branch already exists it will failed
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.
but what if we don't want to create billions of branches though?
commit_message: "fix: update e2e screenshots" | ||
branch: ${{ steps.vars.outputs.BRANCH_NAME }} | ||
file_pattern: ${{ inputs.screenshotsPattern }} | ||
create_branch: true |
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 may want to also create a workflow to automatically delete the e2e-branches when the original branch has been deleted / merged
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.
Could be done in second time I think, you can open an issue to track that proposal
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 think it's better to that in place before we start creating branches, the workflow should not be that hard to write
dddbca5
to
33115a0
Compare
if: steps.update-e2e-screenshots.outputs.screenshots != '0' | ||
uses: stefanzweifel/git-auto-commit-action@e348103e9026cc0eee72ae06630dbe30c8bf7a79 # v5.1.0 | ||
with: | ||
commit_message: "fix: update e2e screenshots" |
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.
commit_message: "fix: update e2e screenshots" | |
commit_message: "test: update e2e screenshots" |
or chore
since it's not actually a code fix.
Also, can you add something to the commit not to have the same message everytime (like the input branch)
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.
It's a temporary commit
you have to cherry-pick it in your PR and squash it with your original commit
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.
but you are not forced to squash it :-s
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 are used to accept PR with only 1 commit
33115a0
to
e5f7d4c
Compare
Proposed change
On PR event, create branch with updated screenshots if any and comment the PR to let the contributor know.
Related issues
🚀 Feature resolves #2701