nodejs / admin

Administrative space for policies of the TSC
150 stars 133 forks source link

Install semantic-pull-requests for nodejs/examples #497

Open bnb opened 4 years ago

bnb commented 4 years ago

I'd like to request that the Semantic Pull Request GitHub App (source) be added for nodejs/examples.

The GitHub App is built by a long-time Node.js community member and current Node.js project member (i18n) @zeke.

I've found it useful in helping create consistent and understandable commit logs in projects, especially as they grow. It seems like an ideal tool to use for nodejs/examples because it helps enforce a consistent commit structure, which will helpfully help reduce confusion for those who are trying to learn 💚

zeke commented 4 years ago

👋 happy to help support this if it's what the community wants.

For the uninitiated, I recorded a demo that covers how to set up and use semantic-release and the Semantic Pull Requests GitHub app:

bnb commented 3 years ago

Per the new bot guidelines, here are the permissions the bot requires:

image

also, per the same policy need 2 +1s from both @nodejs/tsc and @nodejs/community-committee

mmarchini commented 3 years ago

+1

out of curiosity, any reasons to have this as a GitHub App instead of an Action (besides it already being implemented as an App)?

bnb commented 3 years ago

That would be a good question for @zeke 👍🏻

zeke commented 3 years ago

Borrowing from https://github.com/zeke/semantic-pull-requests/issues/90#issuecomment-671442448

FWIW, an app works out much better for multiple projects. It's faster than GH actions, doesn't eat up actions runtime quota, easier to configure and so on. There's more you can do with GH actions but doing so again and again across projects becomes tedious after a while.

There is at least one GitHub Action with similar functionality: https://github.com/amannn/action-semantic-pull-request -- but last I checked it still had an issue with surfacing multiple checks on a single PR: https://github.com/amannn/action-semantic-pull-request/issues/5

On a personal note, I would love to see users gravitate toward Actions for validating their semantic pull requests, because maintaining the semantic-pull-requests GitHub App has required a lot of my attention. See https://github.com/zeke/semantic-pull-requests/issues/90 -- that said, I will continue to support the app until a suitable Actions alternative becomes available.

mmarchini commented 3 years ago

what a weird issue :O

Both the Action and the App only check if commits are following https://conventionalcommits.org, right? If so I've been doing that on some projects with the following Action:

---
name: Conventional Commit Linter
on:
  push:
    branches:
      - master
  pull_request:

jobs:
  name: Lint
  runs-on: ubuntu-latest
  steps:
    - uses: actions/checkout@v2
    - name: Use Node.js v12
      uses: actions/setup-node@v1
      with:
        node-version: v12
    - run: npm install
    - run: npx commitlint --from ${{ github.event.pull_request.base.sha }} --to ${{ github.event.pull_request.head.sha }}

I'm probably missing some edge cases or some use cases though.

bnb commented 3 years ago

@mmarchini commits || title for squash merging, and auto-updates when the title changes.

mmarchini commented 3 years ago

Right, was just reading the Action and my example above definitely ignores the Pull Request title. I think the problem is that the https://github.com/amannn/action-semantic-pull-request Action is creating a new Status for each run so it can stay Pending until WIP is removed. It would probably be better to create a new Check Run, since those will be tied to the current Action Check Suite (what a coincidence, I was playing with this yesterday 😅):

const [owner, repo] = process.env.GITHUB_REPOSITORY.split("/");
const eventPayload = JSON.parse(await readFile(process.env.GITHUB_EVENT_PATH));

const { data: checkRun } = await octokit.checks.create({
  repo,
  owner,
  'name': 'Validate Commit',
  'head_sha': eventPayload.pull_request.head.sha
});

https://github.com/nodejs/node-auto-test/blob/master/tools/actions/validate-commit.js#L9-L17. Although I'm not 100% sure this prevents duplicate, but I think it does. Will retry on the node-auto-test repository later.

zeke commented 3 years ago

cc @amann FYI 👋🏼

mcollina commented 3 years ago

+1