Skip to content

Conversation

@iainlane
Copy link
Member

@iainlane iainlane commented Sep 3, 2024

This is a big update. The initial motivation was to support merge_group events but this required some other changes.

We use merge queues in this repository, and CI is currently failing for merges because this action doesn't support the event. We can't ensure that commits going into main have messages in the correct format. We'd like to use release-please to have releases, and this requires conventional commit messages, so this lack of support is blocking us from having a proper release strategy.

When linting for a merge group event, the title is no longer the thing that you want to check. The commits themselves are what will end up in the main branch. Getting the commits which will be pushed from the merge group is a bit tricky. As for PRs, there are multiple strategies which can be used (squash, merge, rebase). There can also be multiple PRs in a merge group. The PRs to be merged might well be behind the target branch and so rebased by GitHub. We need to be able to handle all of these cases in order to lint the correct commits. Essentially we call a GitHub API to figure out what the commits in our merge group branch are. In the case there are multiple PRs in a merge group, each will get a branch based on the previous (imagine a chain). In this case we will lint each PR's commits separately, and each will get its own results. This is all covered by tests. Tests are a new addition to this action too.

Now, the final part. The Typescript rewrite. Trying to add the merge_group event without the help of types proved to be too hard. In order to get more typing support, I switched to using the upstream libraries from the commitlint project too. This broke the build. That's for one main reason: in commitlint configuration files an extends keyword can be given.

{
  "extends": ["@commitlint/config-conventional"]
}

Notice that's a string. It ends up resolving to a require call in the resulting code. node (JavaScript) Actions have to have their dependencies bundled, but the bundler has no way to know to bundle this module, and even if it did the dynamic require would still refer to a module that isn't installed, since it would be in the bundle.

To get around all of this, we're switching off the node runtime and using bun directly. This isn't detectable from the perspective of the user - good - but for working on the action it has a couple of benefits:

  • There is no transpilation/building/bundling step. We can run the Typescript directly. That fixes the above problem.
  • We don't need a build action to ensure we remembered to run yarn build before committing. The need to bundle code and have generated code in the repository is gone.

@iainlane iainlane force-pushed the iainlane/lint-pr-title-merge-group branch 3 times, most recently from aa3e8e9 to 1493338 Compare September 3, 2024 10:27
@iainlane iainlane changed the title test fix(ci): fix Sep 3, 2024
@iainlane iainlane force-pushed the iainlane/lint-pr-title-merge-group branch 5 times, most recently from 6ffcec7 to a4982bc Compare September 3, 2024 10:50
@iainlane iainlane force-pushed the iainlane/lint-pr-title-merge-group branch 7 times, most recently from 29c8e61 to d9c27eb Compare September 17, 2024 16:16
@iainlane iainlane changed the title fix(ci): fix feat(lint-pr-title): rewrite in Typescript and handle merge_group events Sep 17, 2024
@iainlane iainlane force-pushed the iainlane/lint-pr-title-merge-group branch 8 times, most recently from 66eff4c to 205a034 Compare September 17, 2024 16:48
@iainlane iainlane force-pushed the iainlane/lint-pr-title-merge-group branch 3 times, most recently from a04bee7 to a52af87 Compare September 17, 2024 16:59
@iainlane iainlane marked this pull request as ready for review September 17, 2024 17:02
Copy link
Member

@guicaulada guicaulada left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one small nit.

@iainlane iainlane force-pushed the iainlane/lint-pr-title-merge-group branch from a52af87 to fc0443e Compare September 23, 2024 17:29
…vents

This is a big update. The initial motivation was to support `merge_group` events
but this required some other changes.

We use [merge queues] in this repository, and CI is currently failing for merges
because this action doesn't support the event. We can't ensure that commits
going into `main` have messages in the correct format. We'd like to use
[`release-please`][release-please] to have releases, and this requires
conventional commit messages, so this lack of support is blocking us from having
a proper release strategy.

When linting for a merge group event, the title is no longer the thing that you
want to check. The commits themselves are what will end up in the main branch.
Getting the commits which will be pushed from the merge group is a bit tricky.
As for PRs, there are multiple strategies which can be used (squash, merge,
rebase). There can also be multiple PRs in a merge group. The PRs to be merged
might well be behind the target branch and so rebased by GitHub. We need to be
able to handle all of these cases in order to lint the correct commits.
Essentially we call a GitHub API to figure out what the commits in our merge
group branch are. In the case there are multiple PRs in a merge group, each will
get a branch based on the previous (imagine a chain). In this case we will lint
each PR's commits separately, and each will get its own results. This is all
covered by tests. Tests are a new addition to this action too.

Now, the final part. The Typescript rewrite. Trying to add the `merge_group`
event without the help of types proved to be too hard. In order to get more
typing support, I switched to using the upstream libraries from the `commitlint`
project too. This broke the build. That's for one main reason: in `commitlint`
configuration files an `extends` keyword can be given.

```json
{
  "extends": ["@commitlint/config-conventional"]
}
```

Notice that's a string. It ends up resolving to a `require` call in the
resulting code. `node` (JavaScript) Actions have to have their dependencies
bundled, but the bundler has no way to know to bundle this module, and even if
it did the dynamic `require` would still refer to a module that isn't installed,
since it would be in the bundle.

To get around all of this, we're switching off the `node` runtime and using
[`bun`][bun] directly. This isn't detectable from the perspective of the user -
good - but for working on the action it has a couple of benefits:

- There is no transpilation/building/bundling step. We can run the Typescript
  directly. That fixes the above problem.
- We don't need a `build` action to ensure we remembered to run `yarn build`
  before committing. The need to bundle code and have generated code in the
  repository is gone.

[bun]: https://bun.sh
[merge queues]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/merging-a-pull-request-with-a-merge-queue
[release-please]: https://github.com/googleapis/release-please
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.

2 participants