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

Add snapshot releases for PRs #681

Closed
wants to merge 1 commit into from
Closed

Add snapshot releases for PRs #681

wants to merge 1 commit into from

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Nov 29, 2021

Please review this extra carefully from the security point of view. I've gone through:

And I think this is OK - but I would lie if I would say that I fully grasp all the intricacies involved here.

One surprising thing is that:

Workflows will not run on pull_request activity if the pull request has a merge conflict. The merge conflict must be resolved first.

This seems to be counter-intuitive. I understand why this is the case though - it's because those workflows are executed in the context of the merge commit and that cannot be created if there are any conflicts. I'm not sure how to just execute this properly even if there are conflicts 🤷‍♂️

@changeset-bot
Copy link

changeset-bot bot commented Nov 29, 2021

⚠️ No Changeset found

Latest commit: 5bd17c4

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

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

I think this should be fine in terms of not allowing forks to have access since the pull_request event doesn't give access to secrets on forks. https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#accessing-secrets


jobs:
snapshot_release:
if: github.repository == 'atlassian/changesets'
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if github.repository actually means the repo that the pull request is from? I really wouldn't be surprised if it meant the repo that the PR is to.

Copy link
Member Author

@Andarist Andarist Nov 30, 2021

Choose a reason for hiding this comment

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

Good catch, for the pull_request trigger this should be:

Suggested change
if: github.repository == 'atlassian/changesets'
if: github.event.pull_request.head.repo.full_name == 'changesets/changesets'

This will prevent this workflow PR from potentially running when a pull request targets a fork of the Changesets repo. It's a minor thing but I guess still worth doing if this has already been added, right?

@@ -7,25 +7,27 @@ on:

jobs:
release:
if: github.repository == 'atlassian/changesets'
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if: github.repository == 'atlassian/changesets'
if: github.repository == 'changesets/changesets'

@emmatown
Copy link
Member

emmatown commented Dec 4, 2021

Could we setup CodeSandbox CI instead of doing this? We don't need to use the actual sandboxes but it'll publish the packages somewhere where they can be installed locally + it'll even work for PRs from forks. We also totally could use the sandboxes for testing version calculation things and etc. if we wanted to. I'm kind of apprehensive about publishing every commit from PRs to the real npm packages:

  • I get the "this package was just published" emails and since releases happen relatively rarely, it's totally doable for me to check when I get those emails that a release did indeed happen from a PR and the token wasn't somehow taken and a malicious release happened but it wouldn't be practical to check that if it was every commit from local PRs
  • Package managers essentially download every package.json from every version when installing so having a significantly larger number of releases would slow down installs

@Andarist
Copy link
Member Author

Andarist commented Dec 4, 2021

we could try that

We also totally could use the sandboxes for testing version calculation things and etc

I've thought that those had hashes/timestamps in the version but it seems they preserve the version from package.json. That being said - I don't see myself rechecking this, especially that this information should already be somewhat mentioned in the Changesets Bot's comment.

One downside of this approach is that it becomes much less convenient to install those packages.

Package managers essentially download every package.json from every version when installing so having a significantly larger number of releases would slow down installs

Why this is the case? I don't see how this information is useful when installing a package - the only relevant information should be contained in the versions list and the tags list. This is also a somewhat common practice for popular libraries like React, TypeScript, and more (they publish nightly releases and the list of versions is huge by now). Even if what you are saying is true then if it's not a problem for those popular projects I don't see why we'd care about this.

@Andarist
Copy link
Member Author

Andarist commented Dec 7, 2021

This got superseded by #695

@Andarist Andarist closed this Dec 7, 2021
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.

None yet

2 participants