search

1Password / shell-plugins

Seamless authentication for every tool in your terminal.
https://developer.1password.com/docs/cli/shell-plugins/
MIT License
507 stars 163 forks source link

Fix signed commits CI job #381

Closed SimonBarendse closed 9 months ago

SimonBarendse commented 9 months ago

The job was previously ran on "pull_request_target". This runs the job on the base branch (i.e. the main branch of the 1password/shell-plugins repo). Since we want to know if commits are signed on all the commits to be merged (i.e. also on the fork), I have changed this job to run on the merge commit instead.

Relevant documentation

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

Note that the usage example of https://github.com/1Password/check-signed-commits-action also recommends running on the "pull_reqeusts" event.

Use Cases

See https://github.com/1Password/shell-plugins/pull/332 where we run into this in practice. The job is passed, but commits aren't signed.

florisvdg commented 9 months ago

The change from pull_request to pull_request_target was needed to support PR commenting on PRs from forks. Otherwise the GitHub token mounted in the fork pull_request workflow does not have permissions to comment in the upstream repo.

However, that change alone didn't fix the commenting, because pull_request_target breaks the PR number check, which results in the No PR found to scan for commits. log message you're seeing.

There's a PR open in the action repo that in turn fixes that. I'll make sure to also update the examples/docs in the action repo.

SimonBarendse commented 9 months ago

However, that change alone didn't fix the commenting, because pull_request_target breaks the PR number check, which results in the No PR found to scan for commits. log message you're seeing.

There's a https://github.com/1Password/check-signed-commits-action/pull/4 in the action repo that in turn fixes that. I'll make sure to also update the examples/docs in the action repo.

Looks like this didn't work. The job still exists with the same log message.

florisvdg commented 9 months ago

Looks like this didn't work. The job still exists with the same log message.

That's because it's using pull_request_target which is main and thus not this branch. So in practice, any changes made to this particular workflow will be ignored until they're merged to main. (This is why I had to test it with a fork of a fork 😅)