Skip to content

Fix screenshot comparison#15894

Merged
alice-i-cecile merged 7 commits intobevyengine:mainfrom
mockersf:fix-screenshot-comparison
Oct 14, 2024
Merged

Fix screenshot comparison#15894
alice-i-cecile merged 7 commits intobevyengine:mainfrom
mockersf:fix-screenshot-comparison

Conversation

@mockersf
Copy link
Copy Markdown
Member

@mockersf mockersf commented Oct 14, 2024

Objective

Solution

  • Fix the file path
  • Instead of a pull_request_target workflow, keep the examples in the pull_request workflow and add another job that will run once its all completed on a workflow_run event to upload screenshots

Testing

@mockersf mockersf added D-Trivial Nice and easy! A great choice to get started with Bevy A-Build-System Related to build systems or continuous integration C-Testing A change that impacts how we test Bevy or how users test their apps labels Oct 14, 2024
@BenjaminBrienen BenjaminBrienen added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 14, 2024
@mockersf
Copy link
Copy Markdown
Member Author

@BD103 I reverted the parts that used a pull_request_target to move the upload to a workflow_run event which should still have permissions to access secrets

It's still kinda hard to make sure everything will work fine as it uses the workflow definition from main, not from the PR

@BD103 BD103 self-requested a review October 14, 2024 13:23
@BD103 BD103 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Oct 14, 2024
Copy link
Copy Markdown
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though we'll have to see if it works once this gets merged. :)

var artifacts = await github.rest.actions.listWorkflowRunArtifacts({
owner: context.repo.owner,
repo: context.repo.repo,
run_id: ${{github.event.workflow_run.id }},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
run_id: ${{github.event.workflow_run.id }},
run_id: ${{ github.event.workflow_run.id }},

(Nit) Add a space to make it symmetric.

with:
name: screenshots-macos
path: screenshots-macos

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

(Nit) There was an extra newline here :)

@BD103 BD103 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 14, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 14, 2024
Merged via the queue into bevyengine:main with commit bd912c2 Oct 14, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 14, 2024
… upload-artifact action (#15911)

# Objective

- After #15894, creenshot upload now works on linux but still fails on
macOS
- Artifact with screenshot is not prepared correctly:
![Screenshot 2024-10-14 at 21 58
49](https://github.com/user-attachments/assets/8a50613d-aba6-49ee-b9c3-614d77f34ed0)

## Solution

- Try to prepare it like the `upload-artifact` action expects it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Build-System Related to build systems or continuous integration C-Testing A change that impacts how we test Bevy or how users test their apps D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants