CI on PR - Githubissues

nathanblair commented 2 years ago

Refers to #325

I'm unsure if this tackles all the desired behaviors. I made some assumptions about just targeting the make-all workflow for PRs, and kept the push event as well.

I've also not restricted the push event to any branches, but it could make sense to do so.

That would also simplify the ref-determining code in the case of the push-event as well.

Other than that, I tried to tidy up the file a bit. I find some whitespace between workflow steps helps to read yaml more (I look at this stuff a lot all day).

This workflow file should go into effect/be observable on this PR, as this file exists in the default branch. But my experience with GitHub Actions has been somewhat inconsistent in this regard.

EDIT: Ah, yes, this is coming from a fork so the workflow file will need approved first!

nathanblair commented 2 years ago

Ha. And I forgot to handle the proper remote url anyway...

Yep! Back to the checkout action we go!

nathanblair commented 2 years ago

This might all be information maintainers are already aware of but I want to make sure.

Here's compilation of some of the security implications of workflows with PRs:

So long as the maintainers keep a close eye on any workflow file changes, and as long as there remains no need to employ credentials for automation (other than the auto-generated GITHUB_TOKEN), I see no reason to use pull_request_target over pull_request.

I would also like to emphasize that unless otherwise necessary, make sure to keep the GITHUB_TOKEN permissions set to read-only as its basically the last line of defense in case a maintainer approves a rogue workflow run. If confidential information outside of the GITHUB_TOKEN ever is required (e.g. automating pushes to image registries) and/or write access to the repository is needed I would advise coming up with a more robust solution for workflows running from public forks. That's a discussion outside the scope of this current PR though, I think; happy to be involved and share my experience on the matter if desired!

In the case of this repo I'm assuming no secrets currently exist as I don't see any secrets context listed anywhere in the existing workflow files.

But if there's a hastily approved workflow, and the PR author manages to guess the name of an existing secret (for the ORG or for the repo, even if it hasn't been used in workflows yet) correctly, the secret could become compromised.

AkihiroSuda commented 2 years ago

cc @jessfraz

nathanblair commented 2 years ago

https://github.com/genuinetools/img/runs/3432158057?check_suite_focus=true#step:2:456

I'm not positive this is what we want. I'm acclimating to the checkout action across forks 😅

I have what I believe to be a fix in another local branch; I am going to verify some more functionality before merging it back into this one and pushing.

nathanblair commented 2 years ago

It, in fact, makes no difference as far as this workflow is concerned whether the workflow checks out the actions/checkout-created merge commit or is just the latest HEAD commit of the PR branch. The original commit hash is still viewable at this point in the log. If that's good enough for y'all, no sense in modifying it.

It could probably be useful to stick with the actual HEAD commit if you were to use that commit in the PR workflow to associate artifacts around but for now definitely not necessary.

I'm not sure what's going on in the make step. I'm running on a bunch of macOS and Windows machines and can't build natively on Linux atm so I can't troubleshoot fixing it but it does look like the error originated before this PR:

https://github.com/genuinetools/img/actions/runs/1069026687

nathanblair commented 2 years ago

Just wiped a commit that pinned the actions/checkout to the v2 tag. My suggestion would be to pin it at v2 (that Action is still quite active due to its ubiquity), but strictly speaking, not necessarily in the scope of this PR.

genuinetools / img

CI on PR #345