search

fireblade-engine / ecs

A dependency free, lightweight, fast Entity-Component System (ECS) implementation in Swift
MIT License
110 stars 11 forks source link

switches runs-on target to pull-request-target #70

Closed heckj closed 2 weeks ago

heckj commented 2 weeks ago

Description

This change gives PR runs access to secrets - needed for uploading the code coverage details when run from a fork.

Detailed Design

there's a bit of extended conversation on this detail at https://stackoverflow.com/questions/74957218/what-is-the-difference-between-pull-request-and-pull-request-target-event-in-git, but the gist is that with this change, any code invoked through a pull request has access to the repositories' secrets. As far as I could see from the workflows on this repository, the only real notable secret is the CODECOV token for uploading code coverage, so I think this is a potentially meaningful change.

We'll see if it fails the CI though, which I'd expect if that doesn't "do the trick" for the macOS specific builds. In either case, it probably isn't 100% sensible to upload the code coverage 3 times - one for each of the matrix of builds, so that may be worth refactoring to pick one as the "code coverage" build, and do the others from the matrix without attempting to upload code coverage.

Testing

How is the new feature tested?

Pretty much just workin' the CI system in this case, since that's the system being adjusted.

Checklist

heckj commented 2 weeks ago

The macOS tests haven't/didn't run on this - and it's from a fork - so I'm honestly not even sure if it did fix it...

The inciting incident was the PR https://github.com/fireblade-engine/ecs/pull/69 that failed testing through this flow.

ctreffs commented 2 weeks ago

Thanks for your work finding out about this issue - I've decided to go a different route that keeps workflow triggering regardless of the PR target. See https://github.com/fireblade-engine/ecs/pull/71 for details. I will close this PR since the solution should work as well as your code but with less limits to execution.