-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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: buildless ecosystem ci #18525
ci: buildless ecosystem ci #18525
Conversation
/ecosystem-ci run |
commit: |
/ecosystem-ci run |
script: | | ||
const prData = ${{ steps.get-pr-data.outputs.result }} | ||
const url = `https://pkg.pr.new/vite@${prData.commit}` | ||
const response = await fetch(url) |
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 do not yet implement the HEAD
for pkg.pr.new! So here instead of consuming the request body and waiting for it, we just check the status and then the step would be done.
So if it was fetching, it'd be aborted I think.
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.
cc @dominikg
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.
if you await fetch(), the full file has been downloaded already, its just not been parsed yet. of course if pkg.pr.new does not implement head we have to go with full, but should be considered as an optimization. or do you implement the endpoints about package information that the public registry has?
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.
if you await fetch(), the full file has been downloaded already
I did not know that, does not that defeat the purpose of streams (e.g. .body
)?
As far as I know the await on .body
would wait until finishing the fetch and meanwhile give us the result buffer by buffer while fetching.
but should be considered as an optimization
💯
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.
hmm, maybe need to get more coffee and you are right.
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.
Haha, thanks, let me get my own coffee too now 😅
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 do not yet implement the
HEAD
for pkg.pr.new!
Any plan to support it? It sounds interesting.
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.
Any plan to support it? It sounds interesting.
I'll create an issue for it then!
/ecosystem-ci run |
As far as I know this tests the previous version of vite-ecosystem-ci
rather than the new one.
Which makes it requires a merge.
…On Tue, 5 Nov 2024, 13:43 vite-ecosystem-ci[bot], ***@***.***> wrote:
⏳ Triggered ecosystem CI: Open
<https://github.com//vitejs/vite-ecosystem-ci/actions/runs/11681487209>
—
Reply to this email directly, view it on GitHub
<#18525 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJBMICDQVZRRLA6YQFS6JPLZ7CHMDAVCNFSM6AAAAABQ5A7JXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJWGY4TMNBWGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
📝 Ran ecosystem CI on
✅ analogjs, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, storybook, vite-plugin-pwa, vite-plugin-react-swc, vite-setup-catalogue, vitepress, vuepress |
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 code looks good to me 👍
6c13980
to
f3170e2
Compare
f3170e2
to
d6961c2
Compare
@@ -89,7 +223,7 @@ jobs: | |||
prNumber: '' + prData.num, | |||
branchName: prData.branchName, | |||
repo: prData.repo, | |||
commit: prData.commit, | |||
commit: process.env.COLLISION === 'false' ? prData.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.
To pass commit: ''
, I think we need to update these lines to prevent the error fixed by #18778.
https://github.com/vitejs/vite-ecosystem-ci/blob/6adc524597abdf2a0976c0ea2f681faa8632f5ea/.github/workflows/ecosystem-ci-from-pr.yml#L122
https://github.com/vitejs/vite-ecosystem-ci/blob/6adc524597abdf2a0976c0ea2f681faa8632f5ea/.github/workflows/ecosystem-ci-from-pr.yml#L190
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.
yes, let me do so!
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.
here you go! vitejs/vite-ecosystem-ci#339
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.
merged that, thanks!
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.
LGTM!
Thank you for for merging
…On Tue, 10 Dec 2024, 05:51 sapphi-red, ***@***.***> wrote:
Merged #18525 <#18525> into main.
—
Reply to this email directly, view it on GitHub
<#18525 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJBMICEBRH7R4BRLADW4IFT2EZFZHAVCNFSM6AAAAABQ5A7JXWVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJVGU4TGMZSHE3TIMY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
By using the --commit option in vite-ecosystem-ci, we can leverage the already built versions of vite in pkg.pr.new!
Here, after sending the ecosystem comment, we'll check if there's an already built version, if not, we'll try triggering a build. The 🚀 reaction is a sign that the build is happening and we should wait until it's done.
After that, the normal process continues.