upsidr / merge-gatekeeper

Get better merge control
MIT License
85 stars 14 forks source link

Feature request: support for `[ci skip]`-type functionality #58

Closed markrmullan closed 1 year ago

markrmullan commented 1 year ago

Hello, thank you for all your work in building this amazing tool!

I wanted to run an idea by you all before coding it up, to see if it would make it into the main branch, or if it would be better off landing in a fork. Basically, in a repo that uses merge-gatekeeper, I want to be able to add something in my PR's title (such as [skip-mg], or whatever), which immediately causes merge-gatekeeper's check to succeed (e.g. I have a PR that's just updating documentation, or something).

This functionality already exists in GHA: you can skip an entire workflow run by adding a magic value somewhere in either the PR title or commit message: https://docs.github.com/en/actions/managing-workflow-runs/skipping-workflow-runs. Those values are:

The problem is that merge-gatekeeper's design conflicts with those magic keywords. Because merge-gatekeeper is a required status check, if you mark a PR as [skip ci], then merge-gatekeeper itself never runs, and the PR becomes unmergeable, since the required status check was skipped.

The idea would be to have merge-gatekeeper look for a string like [skip-mg], and immediately exit as a success if it detects this string in the commit message / PR title β€” I believe we should have access to at least one of those values in the pull_request context object, so no additional network requests required or anything too complicated. A new special value like [skip-mg] would avoid the aforementioned conflict of e.g. [skip ci].

Please let me know your thoughts, and if this has already been considered. I am happy to spend some time on this if you believe it has the potential to be merged. Appreciate your consideration.

rytswd commented 1 year ago

Hi @markrmullan, thanks for raising this! πŸ₯° I was personally unaware of this magic keyword support, and this is not something we have given any considerations before πŸ€”

Just for us to better understand your intention - Are you suggesting that we should add a special keyword for Merge Gatekeeper such as [skip mg], so that you can get better control of Merge Gatekeeper? We can certainly look to introduce special keyword support as an advanced setup πŸ‘ Perhaps we may need to consider adding an extra feature flag for enabling the special keyword support, though. I believe it's extremely unlikely to get the special keyword triggered by accident, but we would like to ensure Merge Gatekeeper stays in its simple form for most common use cases. Having an extra flag may also allow users to define their own special keywords. It may well be an overkill, so let us think about that a bit more πŸ™

Also, in the doc on these magic keywords, it mentions how you would need a separate commit to trigger required actions without [skip ci] keyword. That means Merge Gatekeeper still won't run when [skip ci] comment is provided. I suppose Merge Gatekeeper could be configured with pull_request_target instead of pull_request event to ensure that it runs in any Pull Request updates. We would need to work on clear documentation on that, but as long as it's made clear from the documentation, all we need is probably the implementation update to handle special keyword(s) ☺️

markrmullan commented 1 year ago

Hey @rytswd , thanks for your quick response! Yes, we are on the same page in terms of the desired end user experience.

I was thinking about this some more, and on second thought, feel like we should able to close this. The exact same desired behavior can simply be implemented outside of merge-gatekeeper entirely by using a GHA conditional. Here's a code snippet of what that looks like, and causes the merge-gatekeeper job/check to succeed without running all of the downstream polling logic:

jobs:
  merge-gatekeeper:
    name: merge-gatekeeper
    runs-on: ubuntu-latest
    steps:
      - name: Run Merge Gatekeeper
        if: github.event_name == 'pull_request' && !contains(github.event.pull_request.title, '[skip-mg]')
        uses: upsidr/merge-gatekeeper@v1
        with:
          token: ${{ secrets.GITHUB_TOKEN }}
          ignored: ...
          ... rest

Perhaps this could be added as some sort of recipe in the documentation, if you think others would find it useful, but based on this somewhat straightforward workaround, I believe we are good to close this feat request without any code changes to merge-gatekeeper itself! Thank you again for your willingness to chat about this!

rytswd commented 1 year ago

Thank you for the detailed write up, @markrmullan! That approach absolutely makes sense, and is probably easier for users to understand without needing to read up on Merge Gatekeeper behaviours ☺️

We are keen to ensure the right UX and ease of use, so these types of requests are extremely meaningful! Although this may be a specific use case, having this ticket is a great documentation by itself, and if we find the need of further clarification with extra document, we will make sure to add something appropriate πŸ‘