golangci-lint-action icon indicating copy to clipboard operation
golangci-lint-action copied to clipboard

Make only-new-issues work with Merge Queues, not just Pull Requests

Open andrewedstrom opened this issue 1 year ago • 2 comments

Welcome

  • [X] Yes, I understand that the GitHub action repository is not the repository of golangci-lint itself.
  • [X] Yes, I've searched similar issues on GitHub and didn't find any.

Your feature request related to a problem? Please describe.

only-new-issues is fantastic, but it currently only works when the GitHub event_name is pull_request. Please add support for the merge_group event name, so that this feature can work with merge queues as well.

Merge queues are a powerful feature of GitHub for teams that merge many pull requests per day. It lets you queue up multiple pull requests to be merged in a row. Adding a PR to the queue triggers a new run of the status checks, and once the required status checks pass, that PR is merged.

Describe the solution you'd like.

We just need only-new-issues to apply when the GitHub event is merge_group. Insrc/run.ts we would change this:

const ctx = github.context
  if (ctx.eventName !== `pull_request`) {
    core.info(`Not fetching patch for showing only new issues because it's not a pull request context: event name is ${ctx.eventName}`)
    return ``
  }

to this:

const ctx = github.context;
if (ctx.eventName !== `pull_request` && ctx.eventName !== `merge_group`) {
  core.info(`Not fetching patch for showing only new issues because it's not a pull request or merge group context: event name is ${ctx.eventName}`);
  return ``;
}

(I'm assuming there would need to be some additional changes to figure out the patch, but nothing too bad)

Describe alternatives you've considered.

My main alternatives are

  1. Fix all linter errors in my project at once (hard)
  2. Disable the linter as a required check.

Additional context.

No response

andrewedstrom avatar Jan 24 '24 19:01 andrewedstrom

@andrewedstrom could you please explain how does the only-new-issues work in your workflow with regular pull_request event? I have some trouble understanding how does it work - here is my open discussion: https://github.com/golangci/golangci-lint-action/discussions/981

Basically I suppose that it should compare whole pull request with the destination branch (as described in the discussion above) but in my case it doesn't seem to work at all - linter doesn't find any issues even having intentional ones.

I'd be glad to receive some feedback. Cheers!

Sortren avatar Feb 19 '24 18:02 Sortren

@Sortren

  1. We make a pull request
  2. This action runs and checks if any of the lines changed in the pull request have linter issues
  3. We add the pull request to a merge queue and the status checks run again (this is the step where we have the problems raised in this issue)
  4. Once status checks pass, the PR is merged into main.

andrewedstrom avatar Feb 22 '24 00:02 andrewedstrom

Our org is running into a similar issue as well. I probably missed something that don't understand about merge groups but this is what I changed. It seems to work but not sure if thats exactly what's needed to support merge groups.

https://github.com/strantalis/golangci-lint-action/commit/f4e979f5b36068d69d340547f66c991dca8b70ef

strantalis avatar Mar 13 '24 23:03 strantalis

@strantalis Did/can you createa PR with those changes, to hopefully get a full review?

flimzy avatar Apr 10 '24 08:04 flimzy

@flimzy I don't have a pr open. I will open one to hopefully get a conversation started.

strantalis avatar Apr 17 '24 18:04 strantalis

~~I create a test repo: I can never trigger the merge_group event.~~

~~https://github.com/golangci/friendly-lamp/blob/main/.github/workflows/merge-group.yml~~

~~Can anyone provide a working example of merge_group/merge queue?~~ ~~Can anyone drive me to a working configuration?~~

EDIT: I found how to trigger the event.

ldez avatar Apr 26 '24 23:04 ldez

Currently, only-new-issues only works for PR (events pull_request and pull_request_target) because it uses the GitHub API to get the diff from a PR (and not because of the if on the event name).

The addition of ctx.eventName !== `merge_group` will not work because merge_group event doesn't provide information about a PR. The PR number can be found by parsing a ref name but this is not the right way because the diff will come from PR information and not from the rebased/merged/squashed branch commits of the queue.

The support of merge_group with only-new-issues requires writing a dedicated implementation, completely different than the PR support, and will require setting fetch-depth to 0 (because a git diff should be done).


Currently, as a workaround to use merge_group, you should:

  • not use only-new-issues but args
  • use 2 conditional steps for golangci-lint
  • set fetch-depth to 0
name: golangci-lint
on:
  pull_request:
  merge_group:

jobs:
  golangci-lint:
    name: lint
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0

      - uses: actions/setup-go@v5
        with:
          go-version: stable
      
      - name: golangci-lint (PR)
        if: ${{ github.event_name == 'pull_request' }}
        uses: golangci/golangci-lint-action@v5
        with:
          version: v1.57
          only-new-issues: true

      - name: golangci-lint (queue)
        if: ${{ github.event_name == 'merge_group' }}
        uses: golangci/golangci-lint-action@v5
        with:
          version: v1.57
          args: --new-from-rev=${{ github.event.merge_group.base_sha }}

ldez avatar Apr 27 '24 02:04 ldez

FYI, since v5.1.0, the events pull_request, pull_request_target, push, and merge_group are supported by default (no workaround needed).

Note: for merge_group, you still need to set fetch-depth to 0.


Examples

For pull requests:

name: golangci-lint
on:
  pull_request:

jobs:
  golangci-lint:
    name: lint
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-go@v5
        with:
          go-version: stable
      
      - name: golangci-lint
        uses: golangci/golangci-lint-action@v5
        with:
          version: v1.57
          only-new-issues: true

For push:

name: golangci-lint
on:
  push:

jobs:
  golangci-lint:
    name: lint
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-go@v5
        with:
          go-version: stable
      
      - name: golangci-lint
        uses: golangci/golangci-lint-action@v5
        with:
          version: v1.57
          only-new-issues: true

For pull requests, and push:

name: golangci-lint
on:
  push:
  pull_request:

jobs:
  golangci-lint:
    name: lint
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-go@v5
        with:
          go-version: stable
      
      - name: golangci-lint
        uses: golangci/golangci-lint-action@v5
        with:
          version: v1.57
          only-new-issues: true

For pull requests and merge queues:

name: golangci-lint
on:
  pull_request:
  merge_group:

jobs:
  golangci-lint:
    name: lint
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0

      - uses: actions/setup-go@v5
        with:
          go-version: stable
      
      - name: golangci-lint
        uses: golangci/golangci-lint-action@v5
        with:
          version: v1.57
          only-new-issues: true

For pull requests, merge queues, and push:

name: golangci-lint
on:
  push:
  pull_request:
  merge_group:

jobs:
  golangci-lint:
    name: lint
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0

      - uses: actions/setup-go@v5
        with:
          go-version: stable
      
      - name: golangci-lint
        uses: golangci/golangci-lint-action@v5
        with:
          version: v1.57
          only-new-issues: true

ldez avatar Apr 29 '24 21:04 ldez

@ldez Workflows seem to be working. Just wanted to say thanks for the help.

strantalis avatar May 08 '24 11:05 strantalis