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.

Sign up for GitHub

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

Jump to bottom

chore: add custom comment with link to playground for pkg.pr.new #14151

Merged
merged 6 commits into from
Nov 5, 2024
Merged

chore: add custom comment with link to playground for pkg.pr.new #14151

merged 6 commits into from
Nov 5, 2024

Conversation

paoloricciuti
Copy link
Member

After we merge sveltejs/svelte.dev#745 it could be cool to have the link to the playground directly in the comment.

This is following pkg.pr.new recommendation to add a custom comment with some modification on my part to make it more useful for us. I'm not entirely sure what would happen on merge but i guess we can always iterate.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 4, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 6f76685

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14151-svelte.vercel.app/

this is an automated message

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024 •
edited
Loading

pkg.pr.new

svelte

pnpm add https://pkg.pr.new/svelte@14151

Open Playground

@sveltejs sveltejs deleted a comment from github-actions bot Nov 4, 2024
@sveltejs sveltejs deleted a comment from github-actions bot Nov 4, 2024
@paoloricciuti paoloricciuti mentioned this pull request Nov 4, 2024
Merged
3 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

Playground

pnpm add https://pkg.pr.new/svelte@14151

@Rich-Harris
Copy link
Member

tweaked the comment but I believe there were some unresolved questions around using pull_request vs pull_request_target, so will leave unmerged for now

@paoloricciuti
Copy link
Member Author

tweaked the comment but I believe there were some unresolved questions around using pull_request vs pull_request_target, so will leave unmerged for now

Yeah we should check what happens on forks or follow @pngwn suggestion of using two workflows. Will check in a bit

@paoloricciuti
Copy link
Member Author

Mmm i don't think we can try what happens...i will implement @pngwn solution just to be safe

@paoloricciuti
Copy link
Member Author

Mmm apparently worflow_run doesn't trigger is it's not on the main branch so we have no way to test this without merging it first.

https://github.com/orgs/community/discussions/25288

What should we do? Merge and eventually fix?

@Rich-Harris Rich-Harris merged commit 0ce4b55 into main Nov 5, 2024
11 checks passed
@Rich-Harris Rich-Harris deleted the pkg-pr-new-custom-comment branch November 5, 2024 13:07
@pngwn
Copy link
Member

pngwn commented Nov 5, 2024

I think this will fail. It is referencing a context value that doesn’t exist in this event type.

@paoloricciuti
Copy link
Member Author

I think this will fail. It is referencing a context value that doesn’t exist in this event type.

Which context doesn't exist here? I saved, sha, and issue number on the output itself... isn't context.repo.owner still available?

@Rich-Harris
Copy link
Member

It failed, but for a different reason: https://github.com/sveltejs/svelte/actions/runs/11684845805

pngwn
pngwn reviewed Nov 5, 2024
View reviewed changes
.github/workflows/pkg.pr.new-comment.yml

if (output.event_name === 'pull_request') {
if (context.issue.number) {
await create_or_update_comment(context.issue.number);
Copy link
Member

Choose a reason for hiding this comment

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

@paoloricciuti This

Copy link
Member

Choose a reason for hiding this comment

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

Actually it will never even run looking at this. None of these events types match.

Copy link
Member Author

Choose a reason for hiding this comment

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

The event name is on the output not on the context...but yeah this I missed it...will fix

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. Makes sense.

@pngwn
Copy link
Member

pngwn commented Nov 5, 2024

I would check that the artifact is in the right place. Or where we are reading it from is correct. Helpful to ls files when working with artifacts.

@paoloricciuti
Copy link
Member Author

I would check that the artifact is in the right place. Or where we are reading it from is correct. Helpful to ls files when working with artifacts.

Yeah that's the problem probably...if I push code that changes the workflow in a new PR will it run in the context of the pr?

@pngwn
Copy link
Member

pngwn commented Nov 5, 2024

Not on the workflow run workflow but you could check the location on the pr workflow after creating the artifact. That might offer some insight.

@pngwn
Copy link
Member

pngwn commented Nov 5, 2024

I would leave off the path tbh. The default should be fine. Then I think the existing code might work.

It could be that it is outputting to output.json/output.json

trueadm pushed a commit that referenced this pull request Nov 7, 2024
@paoloricciuti @Rich-Harris @trueadm
chore: add custom comment with link to playground for pkg.pr.new (#…
ec57f44 
…14151)

* chore: add custom comment with link to playground for `pkg.pr.new`

* chore: small updates to the script output

* chore: refine comment a bit, include multiple packages

* tweak comment

* chore: two steps workflow

* chore: update workflow

---------

Co-authored-by: Rich Harris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pngwn pngwn pngwn left review comments

@Rich-Harris Rich-Harris Rich-Harris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paoloricciuti @Rich-Harris @pngwn