Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting merge method through labels #92

Merged
merged 5 commits into from
Aug 2, 2020
Merged

Allow setting merge method through labels #92

merged 5 commits into from
Aug 2, 2020

Conversation

michaelbeaumont
Copy link
Contributor

Hi, this PR is meant also as a jumping off point for discussion around the idea. I'm not that attached to the exact names but using labels like this seems like a ergonomic, backwards compatible way to support changing the merge method for specific PRs.

@karfau
Copy link

karfau commented Jul 11, 2020

Something like what the title suggest would be awesome!
Currently our "workaround" is

jobs:
  detect:
    runs-on: ubuntu-latest
    steps:
      - id: detectPR
        uses: "bettermarks/gh-find-current-pr@bettermarks"
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}
      - name: Automerge
        if: success() && steps.detectPR.outputs.label_automerge
        uses: "pascalgn/automerge-action@master"
        env:
          GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
          MERGE_DELETE_BRANCH: "true"
      - name: Autorebase
        if: success() && steps.detectPR.outputs.label_autorebase
        uses: "pascalgn/automerge-action@master"
        env:
          GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
          MERGE_LABELS: "autorebase"
          MERGE_METHOD: "rebase"
      - name: Autosquash
        if: success() && steps.detectPR.outputs.label_autosquash
        uses: "pascalgn/automerge-action@master"
        env:
          GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
          MERGE_LABELS: "autosquash"
          MERGE_METHOD: "squash"

Which also means that the mentioned gh-find-current-pr action needs to have all the same logic finding the correct PR based on the events, which is tricky, not only from a maintenance perspective.

Personally I would still prefer that it's a new feature that is enabled via an option,

  • so backward compatibility is not a concern
  • it would be possible to allow just setting one label for that feature (I would expect most people only using one "required" label), like in my example above: automerge, autosquash, autorebase. (Update I initially misunderstood the implementation, it's actually possible to just use one label, see below.)

@michaelbeaumont
Copy link
Contributor Author

  1. A flag makes sense
  2. Can you clarify what you mean by one label? With this PR one would only need apply e.g. automerge.squash or <required>.<type>

@karfau
Copy link

karfau commented Jul 14, 2020

Just checked your code again and realized that I misunderstood it when I first read it.
I understood that the .method label would be required in addition.
But that was my mistake in reading the code.
So just ignore that part.

Copy link

@karfau karfau left a comment

Choose a reason for hiding this comment

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

I love it :)

@pascalgn
Copy link
Owner

pascalgn commented Jul 15, 2020

Hey, sorry for the delay!

I think this is a nice feature, but I know that there are several people who use highly customized configuration values, so forcing the fixed merge/rebase/squash suffixes feels wrong to me.

Instead, my suggestion would be the following, although maybe not ideal, either:

  1. change MERGE_LABELS option to support ? prefix, meaning when using it like MERGE_LABELS: "done,?automerge/autorebase/autosquash,!wip" it would mean that a PR is only merged if 1. the done label is present, 2. the wip label is not present and 3. exactly one of the automerge, autorebase or autosquash labels is present
  2. change MERGE_METHOD to support the configuration like automerge=merge,autorebase=rebase,autosquash=squash

Strictly speaking, this (the ? prefix) would be a breaking change, but I am being a bit naive here and just hope that nobody currently uses a label like ?some-label right now

I think with these 2 changes, we will keep the flexibility of the action, while still achieving the goal of this PR. Unfortunately, I think this is a bit more effort than the current PR, but I think it would be worth it.

Let me know what you think!

@michaelbeaumont
Copy link
Contributor Author

michaelbeaumont commented Jul 15, 2020

@pascalgn

  1. What if we have MERGE_METHOD_LABEL_REQUIRED=(true|false) instead of forcing the user to write ?automerge/autorebase/autosquash? Unless you want the "exactly one of" option in general.
  2. What about leaving MERGE_METHOD as is (before the PR) and adding MERGE_METHOD_LABELS with the format you suggested? Then we have MERGE_METHOD be the default if one of the MERGE_METHOD_LABELS isn't present.

@TGTGamer
Copy link

TGTGamer commented Jul 19, 2020

Just wanted to throw my hat in the loop, would love to see this.

MY current work around is to use env variables which are set using something like:

- run  echo ::set-env name=mergeType::"squash"

and when paired with a standard if statement, allows for both a complex and quick solution.

Something like what the title suggest would be awesome!
Currently our "workaround" is

jobs:
  detect:
    runs-on: ubuntu-latest
    steps:
      - id: detectPR
        uses: "bettermarks/gh-find-current-pr@bettermarks"
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}
      - name: Automerge
        if: success() && steps.detectPR.outputs.label_automerge
        uses: "pascalgn/automerge-action@master"
        env:
          GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
          MERGE_DELETE_BRANCH: "true"
      - name: Autorebase
        if: success() && steps.detectPR.outputs.label_autorebase
        uses: "pascalgn/automerge-action@master"
        env:
          GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
          MERGE_LABELS: "autorebase"
          MERGE_METHOD: "rebase"
      - name: Autosquash
        if: success() && steps.detectPR.outputs.label_autosquash
        uses: "pascalgn/automerge-action@master"
        env:
          GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
          MERGE_LABELS: "autosquash"
          MERGE_METHOD: "squash"

So my version of this would look like:

 jobs:
     detect:
    runs-on: ubuntu-latest
    steps:
      - id: detectPR
        uses: "bettermarks/gh-find-current-pr@bettermarks"
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}
      - run: |
         if [ ${{steps.detectPR.outputs.label_autorebase}} == false ]; then echo ::set-env name=mergeType::"squash" ; 
         else echo ::set-env name=mergeType::"rebase" ; fi
      - name: Automerge
        if: success()
        uses: "pascalgn/automerge-action@master"
        env:
          GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
          MERGE_DELETE_BRANCH: "true"
          MERGE_METHOD: ${{env.mergeType}}

@pascalgn
Copy link
Owner

  1. What if we have MERGE_METHOD_LABEL_REQUIRED=(true|false) instead of forcing the user to write ?automerge/autorebase/autosquash? Unless you want the "exactly one of" option in general.

Not sure about this one. How would it be possible to customize the labels then, if someone wants to have e.g. merge/rebase/squash as labels?

  1. What about leaving MERGE_METHOD as is (before the PR) and adding MERGE_METHOD_LABELS with the format you suggested? Then we have MERGE_METHOD be the default if one of the MERGE_METHOD_LABELS isn't present.

Yes, that's better than what I suggested! 👍

@michaelbeaumont
Copy link
Contributor Author

@pascalgn Pushed the changes including the 2 points I mentioned.

lib/merge.js Outdated Show resolved Hide resolved
lib/common.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@pascalgn pascalgn merged commit 7da9b1b into pascalgn:master Aug 2, 2020
@pascalgn
Copy link
Owner

pascalgn commented Aug 2, 2020

Thanks for the PR, it's merged now! 👍

@michaelbeaumont michaelbeaumont deleted the merge_method_labels branch August 2, 2020 14:20
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.

4 participants