Improve compatibility of `is-release` job condition

[Gudahtt]

The is-release job condition now includes a check for the event trigger type, preventing it from running on pull_request events. This ensures we only release from the main branch, and it acts as further protection against running the action-is-release action in response to a pull_request event (it fails with a confusing error in that situation).

The comment has been improved as well.

Original description:

The is-release job condition is now compatible with more release workflows. Previously it assumed that release commits were made by GitHub Actions, but that's not true anymore in some of our repositories.

Instead, we now ensure just that the triggering event was a push event. push events are only triggered on this workflow for the main branch, so that will omit any events from PRs. It also ensures that the action-is-release action is not triggered for pull_request events, which it does not currently support.

action-is-release should correctly identify releases, so long as it's only run on the main branch for push events in a repository that only allows Squash & merge commits on the main branch. These requirements have been explained in a comment.

[Gudahtt]

This is follow-up from a similar change recently made in the controllers repo: https://github.com/MetaMask/controllers/pull/981#issuecomment-1320660129

[Gudahtt]

Putting this back into draft for now. @rickycodes pointed out that this was not as safe as we'd like it to be; this condition could allow for accidental/unintended triggering of the release workflow. We'll work on an additional condition to use as the trigger.

[Gudahtt]

So the issue here is that any change to the top-level manifest version could now trigger a release. There might be situations where we want to change the version without releasing something, or a version change might sneak its way into a PR unnoticed (this has happened before, with contributors thinking they should handle the release changes in the same PR as the change).

Ideally we'd have another condition to filter down to just the "true" releases. Previously the author worked well enough for this, because we only used that commit author for release PRs. But now that's out.

Using the branch name might work well, but that doesn't seem to be available from a push event when we're using Squash & Merge.

[mcmire]

Using the branch name might work well, but that doesn't seem to be available from a push event when we're using Squash & Merge.

It sounds like github.event.pull_request.head.sha lets us access the latest commit id of the original branch. Could we essentially run git name-rev ${{ github.event.pull_request.head.sha }} | sed -E 's/(~[0-9]+)?$// to get the branch name (could leave the sed off if we are confident enough we don't need it)?

[Gudahtt]

github.event.pull_request is only set for pull_request events. We trigger releases with the push event on the main branch. We changed this so that it could be compatible with the npm-publish environment being restricted to the main branch, to ensure the publish key was protected by our branch protection settings (pull_request triggers in the context of the branch being merged, even if the trigger is for when it's merged/closed).

[Gudahtt]

One possible solution: https://github.com/MetaMask/action-is-release/pull/4

[mcmire]

github.event.pull_request is only set for pull_request events. We trigger releases with the push event on the main branch.

Er, right. I knew that 😂

So the issue here is that any change to the top-level manifest version could now trigger a release. There might be situations where we want to change the version without releasing something, or a version change might sneak its way into a PR unnoticed (this has happened before, with contributors thinking they should handle the release changes in the same PR as the change).

Okay, let's step through this:

• Given that it is impossible to create a commit on main and push it directly due to branch protection rules, the only place that a version bump to the root package could occur is within a PR based on main.
• According to the current logic, such a PR is de facto a release PR.
• Anyone can create such a PR, contributors included. Only a person on our team can merge such a PR, but if that PR contains a version bump to the root package and that person doesn't realize this, they would be surprised when merging such a PR would kick off the release workflow.
• Therefore, we need a way to control when the release workflow is launched, and based on this scenario, it seems that continuing to use the original author of the PR (as when we were using action-create-release-pr) is the most robust indicator (one that could not be spoofed either intentionally or unintentionally).
• Proposal: We add logic which would prevent launching the release workflow when a PR is merged that was created by someone who is not within @MetaMask/devs (or whatever group is appropriate).
• Relating this back to commits, when a push to main occurs, we would look through the commits pushed (github.event.commits), and if there are any commits that were authored or committed by someone outside of our group, we would not launch the release workflow, otherwise we would.

Would that work?

[Gudahtt]

The main problem with that idea is that it doesn't protect against someone within @MetaMask/devs from accidentally changing the release version. We have had multiple examples of this happening already (mostly new team members not used to our release process, or older team members accustomed to how we made library changes before we implemented the current process.).

The commit author being on our team doesn't alone signal that the intention was for a release to be created.

And as for protecting against third-parties triggering releases, the commit author is spoofable, so it doesn't protect against that either.

[mcmire]

Ah, you're right. I guess everything is ultimately spoofable, with varying degrees of difficulty. Your solution in is-release seems like a good one.

[Gudahtt]

I'm reconsidering this change. The original filter does seem appropriate for any repo that is using the create-release-pr workflow, which this template is.

I'll update this PR to just add the push filter, and leave the other filter in place.

We had to remove this filter in the controllers repo because we're not using create-release-pr there (we're using the create-release-branch CLI tool instead, which has support for independently-versioned monorepos). We can consider the commit-message-based filter there instead, and any other place where we use this CLI tool.

[Gudahtt]

Ok, the PR has been updated to just add the trigger filter, and improve the comment.