Skip to content
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

ci: fix build-head size failure for forks #705

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

schirrel
Copy link
Contributor

@schirrel schirrel commented Jan 26, 2025

πŸ”— Linked issue

Not a related issue but this state of Dan about the failure of the build-head from size for forks
#702 (comment)

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Adding repository: ${{ github.event.pull_request.head.repo.full_name }} to comparate build size for forked branchs too
From actions/checkout#551

@schirrel schirrel requested a review from danielroe as a code owner January 26, 2025 03:07
@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2025

Codecov Report

All modified and coverable lines are covered by tests βœ…

Please upload report for BASE (main@4b9141e). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #705   +/-   ##
=======================================
  Coverage        ?   13.33%           
=======================================
  Files           ?       66           
  Lines           ?     3351           
  Branches        ?       95           
=======================================
  Hits            ?      447           
  Misses          ?     2873           
  Partials        ?       31           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

Copy link

pkg-pr-new bot commented Jan 26, 2025

Open in Stackblitz

npm i https://pkg.pr.new/create-nuxt-app@705
npm i https://pkg.pr.new/@nuxt/cli@705
npm i https://pkg.pr.new/nuxi@705

commit: febbde7

@schirrel schirrel changed the title ci: fix build-head siize failure for forks ci: fix build-head size failure for forks Jan 26, 2025
@schirrel schirrel force-pushed the ci/build-head-failure-for-forks branch from 577a44f to fb88def Compare January 26, 2025 03:45
Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

thank you πŸ™β€οΈ

@danielroe
Copy link
Member

oops. too hasty an approval.

still, thank you for looking at this

@schirrel
Copy link
Contributor Author

THanks @danielroe , i am tring to see why the compare is failing, looks like it is relative to permisison/fork too, i am investigating that

@schirrel
Copy link
Contributor Author

@danielroe i've tried one hour long all the things to find out why it was not working... even tho i was wondering if it is related to the fork, i saw some olders PR (weeks not that much) that the check passed.
Please fill free to close this one and create a branch with cherry-pick/copying the code to check if it all pass.
SOrry about that, thanks to look it up

@danielroe
Copy link
Member

thank you! if you're happy to leave this PR open, I can experiment a bit with it

@schirrel
Copy link
Contributor Author

@danielroe for sure, feel free to play around with it

@schirrel schirrel force-pushed the ci/build-head-failure-for-forks branch from 1641a70 to ec73171 Compare January 26, 2025 09:47
@schirrel
Copy link
Contributor Author

I did some testing and looks like when changing from pull_request to pull_request_target works, but the pull_request_target must be on the main yml.
At my test it does look like related to the fork, but quite doesnot make sense to me, once other prs with fork have passed on the past

@schirrel
Copy link
Contributor Author

@danielroe sorry i wrote it wrong, by:

but the pull_request_target must be on the main yml.

i mean on the yml of the main branch here https://github.com/nuxt/cli/blob/main/.github/workflows/size.yml
at my tests, adding it on the branch/pr yml did not make any effect :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants