-
-
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
chore: pkg.pr.new preview with label and comment #18211
chore: pkg.pr.new preview with label and comment #18211
Conversation
Run & review this pull request in StackBlitz Codeflow. |
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.
Thanks for the fix @Aslemammad! I'll let it open in case someone else wants to check it out. Then we can merge and test it out
- run: | | ||
gh issue edit ${{ github.event.issue.number }} --add-label "pr: preview" --repo ${{ github.repository }} | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
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.
Might this have an issue with the default token limitation @userquin was mentioning in vitest-dev/vitest#6362 (comment)?
Actually, github actions can also add labels but labels added via their default bot cannot trigger other actions. 🫠 That's why add it using separate user.
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, interesting, didn't know that! isn't there any workaround to this?
because adding custom tokens might not feel that good!
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 there's no solution, maybe we can go with the manual label addition!
cc @patak-dev
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.
Ya, I think the Vitest setup may work well for Vite too.
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.
You can use a bot, check vitepress, ask @brc-dd
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.
just made the change!
"trigger: preview" would cause a publish. Then that label would be removed for next time runs, so they'd be caused by adding the same label again.
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 added "trigger: preview" label, but it looks like the action is skipped https://github.com/vitejs/vite/actions/runs/11081250725/job/30792784818?pr=18211. I guess this is because of new workflow is coming from a fork repo?
I'm not sure but it looks okay to test it out after merging this.
Why can't the action checkout the PR branch using a flow/logic like this? actions/checkout#331 |
I think that's what's brought up in stackblitz-labs/pkg.pr.new#187 stackblitz-labs/pkg.pr.new#200. It looks like there's some issue with webhook metadata, which we cannot workaround from action side. |
I still don't quite understand the limitation. The issue_comment event has a way to get the PR number, pkg.pr.new should be able to grab the information / latest sha and publish from there. Many existing preview releases workflows that I've seen also don't really need a two step workflow to fix this. I think it's something pkg.pr.new should look deeper into instead of accepting this workaround IMO. |
This is a limitation in pkg.pr.new! With relying on github webhook data, we won't need to deal with issues like spam or authentication. Authentication is just a hash of some unique (temporary) info provided by the webhook, and if it was replicated by the cli, then the publish is authenticated. the issue_comment event in the webhook does not give us the information needed for generating that hash so we can later replicate it in a workflow run. But in the soon future, we're thinking about a refactor where we'd stop relying on github so we'd support other environments, this will hopefully help us navigate the issue_comment problem. |
Or maybe because this my first ever PR in vite that it won't allow running any workflow. |
Hey folks, I wonder if we can get this merged! Let me know if I should try anything before merging this. |
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.
Let's try this approach for a bit. To summarize, every time we'll add a trigger: preview
label, a preview will be issued for the PR. I'm also good with the approach that vitest ended up using (every time you add a cr-tracked
label, every commit after it in the PR gets published). We can check what works better and align later on.
And thanks for your help to keep exploring the diff approaches with us @Aslemammad! |
Description
issue comment events are not proper for pkg.pr.new since they do not pass the right information around the pull request we are running.
This causes few issues, like publishing vite versions from the last state of the main branch rather than the current PR branch.
What this PR does is it uses a workaround to leverage issue comment events. A comment would create a label, which would trigger the pull request labeled event and then it'd run the workflow.
After the run, it'd remove the label so it leaves the opportunity to other comments to cause a preview for the PR.