Conversation
|
8bc2185 to
5bd17c4
Compare
emmatown
left a comment
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch, for the pull_request trigger this should be:
| 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?
|
|
||
| jobs: | ||
| release: | ||
| if: github.repository == 'atlassian/changesets' |
There was a problem hiding this comment.
| if: github.repository == 'atlassian/changesets' | |
| if: github.repository == 'changesets/changesets' |
|
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:
|
|
we could try that
I've thought that those had hashes/timestamps in the version but it seems they preserve the version from One downside of this approach is that it becomes much less convenient to install those packages.
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. |
|
This got superseded by #695 |
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:
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 🤷♂️