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

Fix signed commits CI job #381

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Fix signed commits CI job #381

merged 4 commits into from
Sep 22, 2023

Conversation

SimonBarendse
Copy link
Member

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 #332 where we run into this in practice. The job is passed, but commits aren't signed.

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.
@florisvdg
Copy link
Member

florisvdg commented Sep 22, 2023

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.

The previously version of the check-signed-commits-action did not
support forks. So bumping to the latest version which does support
forks.

I've switched to @main so we automatically keep up with new updates
to the check-signed-commits-action without action in this repo
required.
@SimonBarendse
Copy link
Member Author

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 1Password/check-signed-commits-action#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.

Copy link
Member

@florisvdg florisvdg left a comment

Choose a reason for hiding this comment

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

Usually not a fan of using @main in actions, but given that we own both repos I think it's acceptable in this case.

@florisvdg
Copy link
Member

florisvdg commented Sep 22, 2023

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 😅)

We don't use the checked out code, so this is a redundant step in
this job.
@SimonBarendse SimonBarendse merged commit 73066ed into main Sep 22, 2023
5 checks passed
@SimonBarendse SimonBarendse deleted the simon/fix-commit-sign-check branch September 22, 2023 11:53
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.

3 participants