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

Action fails on PRs that are not backported #127

Open
mrgrain opened this issue May 15, 2024 · 19 comments · May be fixed by #168
Open

Action fails on PRs that are not backported #127

mrgrain opened this issue May 15, 2024 · 19 comments · May be fixed by #168

Comments

@mrgrain
Copy link

mrgrain commented May 15, 2024

Using the example workflow in the readme, the action will fail when a PR is merged that does not have a backport label.
This is functionally correct, but causes confusion because the PR is now displayed with a ❌ (red x) next to it.

Do you have any suggestion how this could be avoided?
To me it feels like the Action should succeed because the PR does not have any relevant labels attached. But I am open to other suggestions.

Here is the error:

  "error": {
    "name": "BackportError",
    "errorContext": {
      "code": "no-branches-exception"
    }
  },
  "errorMessage": "There are no branches to backport to. Aborting."

Example Workflow run: https://github.com/aws/jsii-rosetta/actions/runs/9088130104/job/24977165701
Example PR: aws/jsii-rosetta#1359

@Achllle
Copy link

Achllle commented Jun 12, 2024

I agree with this. Also when the PR hasn't been merged, the check will fail, which is also not great if you're requiring all checks to have passed to allow merging. Adding a simple condition to not run in those cases should suffice, like is done in tibdex/backport.

@mrgrain
Copy link
Author

mrgrain commented Jun 12, 2024

I agree with this. Also when the PR hasn't been merged, the check will fail, which is also not great if you're requiring all checks to have passed to allow merging. Adding a simple condition to not run in those cases should suffice, like is done in tibdex/backport.

Thanks for the link! I've already had a condition:
https://github.com/aws/jsii-rosetta/blob/6d6cf286dee0d344c1a52c4c620e02f1093ed6b7/.github/workflows/backport.yml#L16

But using the github.event.label.name to check the name of the actual label is a good idea!

@Achllle
Copy link

Achllle commented Jun 12, 2024

Consider making a PR since it's a good default 😉 If not, I can make one

Krzmbrzl added a commit to Krzmbrzl/backport-github-action that referenced this issue Jul 6, 2024
This should prevent users from experiencing sorenlouv#127 when simply following the README example.
sorenlouv pushed a commit that referenced this issue Jul 10, 2024
* Prevent example from failing on unmerged PRs

This should prevent users from experiencing #127 when simply following the README example.

* Also don't run README example on backport PRs
@mrgrain
Copy link
Author

mrgrain commented Jul 30, 2024

Finally had time to get back to this. Basically github.event.label.name doesn't work either. The issue is with the pull_request.closed event, when no matching labels are set. That always causes this action to fail.

Unfortunately there is no native to have a complex check for labels in GH Action expressions.

The simple case works:

contains(github.event.pull_request.labels.*.name, 'my-label')

But the other cases don't really

  • my-label-a and my-label-b could be represented as a long chain of ORs (||), but that's annoying and will have a limit
  • startsWith('my-label-') cannot be done.

tl;dr I have added a custom bash step to check labels

jobs:
  backport:
    name: Backport PR
    runs-on: ubuntu-latest
    permissions: {}
    if: github.event.pull_request.merged == true
    steps:
      - name: Check for backport labels
        id: check_labels
        run: |-
          labels='\${{ toJSON(github.event.pull_request.labels.*.name) }}'
          matched=$(echo $labels | jq '. | map(select(startswith("my-label-"))) | length')
          echo "matched=$matched"
          echo "matched=$matched" >> $GITHUB_OUTPUT
      - name: Backport Action
        if: fromJSON(steps.check_labels.outputs.matched) > 0
        uses: sqren/backport-github-action@v9.5.1

@Achllle
Copy link

Achllle commented Jul 31, 2024

@mrgrain I can confirm the fix in #133 works and didn't need any custom bash scripting, ICYMI.

@mrgrain
Copy link
Author

mrgrain commented Jul 31, 2024

@mrgrain I can confirm the fix in #133 works and didn't need any custom bash scripting, ICYMI.

What #133 changed is exactly what I had before and doesn't seem to fix it for us.

@jfagoagas
Copy link

@mrgrain I think there is a typo in your action file. There is a \ within the labels variable in the check_labels step.

This workaround works fine for me:

jobs:
  backport:
    name: Backport PR
    if: github.event.pull_request.merged == true && !(contains(github.event.pull_request.labels.*.name, 'backport'))
    runs-on: ubuntu-latest
    steps:
      # Workaround not to fail the workflow if the PR does not need a backport
      # https://github.com/sorenlouv/backport-github-action/issues/127#issuecomment-2258561266
      - name: Check for backport labels
        id: check_labels
        run: |-
          labels='${{ toJSON(github.event.pull_request.labels.*.name) }}'
          matched=$(echo "${labels}" | jq '. | map(select(startswith("backport-to-"))) | length')
          echo "matched=$matched"
          echo "matched=$matched" >> $GITHUB_OUTPUT

      - name: Backport Action
        if: fromJSON(steps.check_labels.outputs.matched) > 0
        uses: sorenlouv/backport-github-action@v9.5.1
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}
          auto_backport_label_prefix: backport-to-

      - name: Info log
        if: ${{ success() && fromJSON(steps.check_labels.outputs.matched) > 0 }} 
        run: cat ~/.backport/backport.info.log

      - name: Debug log
        if: ${{ failure() && fromJSON(steps.check_labels.outputs.matched) > 0 }}
        run: cat ~/.backport/backport.debug.log

Thank you!

@mrgrain
Copy link
Author

mrgrain commented Aug 9, 2024

Thanks for fixing the typo! Oh and I like the debug logging! @jfagoagas

@jfagoagas
Copy link

jfagoagas commented Aug 9, 2024

Thanks for fixing the typo! Oh and I like the debug logging! @jfagoagas

It's nothing! The debug logging wasn't my doing, all credits to @sorenlouv https://github.com/sorenlouv/backport-github-action?tab=readme-ov-file#getting-started

@jfagoagas
Copy link

jfagoagas commented Sep 26, 2024

I don't know if this is related but I have some failing jobs due to the fact that once the backport-to- label is added to the PR and then that PR is merged the following occurs:

  1. The action runs to create the backport PR and adds the was-backported label to the original PR.
  2. The action runs again since the original PR was closed and labeled, with the was-backported, but this time the action fails with the following error "There are no branches to backport to. Aborting." since there is no need to create a backport because there is one already created.

Probably I'm doing something wrong but just wanted to double check if anyone has had this problem before.

@Krzmbrzl
Copy link
Contributor

I think this issue can be resolved by use of https://github.com/marketplace/actions/label-checker-for-pull-requests (this also avoids reinventing the wheel in every workflow file just to be able to check some labels)

@jfagoagas
Copy link

That's perfect @Krzmbrzl 👏

Updated workflow:

name: Automatic Backport

on:
  pull_request_target:
    branches: ['main']
    types: ['labeled', 'closed']

jobs:
  backport:
    name: Backport PR
    if: github.event.pull_request.merged == true && !(contains(github.event.pull_request.labels.*.name, 'backport'))
    runs-on: ubuntu-latest
    steps:
      - name: Check labels
        id: preview_label_check
        uses: docker://agilepathway/pull-request-label-checker:v1.6.55
        with:
          allow_failure: true
          prefix_mode: true
          one_of: backport-to-v
          none_of: was-backported
          repo_token: ${{ secrets.GITHUB_TOKEN }}

      - name: Backport Action
        uses: sorenlouv/backport-github-action@v9.5.1
        if: steps.preview_label_check.outputs.label_check == 'success'
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}
          auto_backport_label_prefix: backport-to-

      - name: Info log
        if: ${{ success() && steps.preview_label_check.outputs.label_check == 'success' }} 
        run: cat ~/.backport/backport.info.log
        
      - name: Debug log
        if: ${{ failure() && steps.preview_label_check.outputs.label_check == 'success' }}
        run: cat ~/.backport/backport.debug.log        

@Krzmbrzl
Copy link
Contributor

Krzmbrzl commented Oct 2, 2024

@jfagoagas do you want to create a PR updating the example in the README or shall I do it? :)

@jfagoagas
Copy link

jfagoagas commented Oct 2, 2024

I'll do that, also I'm wondering about adding the following to avoid running the action for the backport and backported PRs, what do you think?

-    if: github.event.pull_request.merged == true && !(contains(github.event.pull_request.labels.*.name, 'backport'))
+    if: github.event.pull_request.merged == true && !(contains(github.event.pull_request.labels.*.name, 'backport')) && !(contains(github.event.pull_request.labels.*.name, 'was-backported'))

@jfagoagas jfagoagas linked a pull request Oct 2, 2024 that will close this issue
@Krzmbrzl
Copy link
Contributor

Krzmbrzl commented Oct 2, 2024

I think that requiring the presence of a auto-backport-to-xxx label on a PR should be sufficient (together with requiring the PR to be merged). How large is the probability that such a label is assigned to a backport PR? Also, I think it rather unlikely that an already backported PR is relabelled again which could trigger the backport action. And even if it does the maintainer can simply close that second backport PR.

In other words: I am not sure whether going this far with "negative label checks" is actually necessary 🤔

Finally, I am wondering whether a dedicated job for the label checking might make things more concise as that would only require a single if clause.
Something like I am currently (trying to) doing at
https://github.com/mumble-voip/mumble/blob/master/.github/workflows/backport.yml

(There are still some issues with how exactly this is being set up though, so it is not yet working)

@jfagoagas
Copy link

jfagoagas commented Oct 2, 2024

@Krzmbrzl The issue I'm having is that the PRs that needs to be backported, the ones having the backport-to-* label, are also labeled as was-backported once the action runs so that label needs to be excluded too because the workflow is triggered when a PR is labeled, so I think that the above is needed.

So, the action needs to run when all of the following happens:

  1. The PR is merged. -> Done with the if at the job level.
  2. The PR does not contains the label backport -> Done with the if at the job level.
  3. The PR does not contains the label was-backported -> Done with the if at the job level.
  4. The PR contains the label backport-to (or the tag of your choice) -> Done with the preview_label_check step.

@Krzmbrzl
Copy link
Contributor

Krzmbrzl commented Oct 3, 2024

I don't think 2) is necessary as backport PRs should not get the backport-to label, do they?

@jfagoagas
Copy link

Hi @Krzmbrzl you are right but with that you'll skip the execution of the whole action, removing that you will get to the label check where the action will stop.

@Krzmbrzl
Copy link
Contributor

Krzmbrzl commented Oct 4, 2024

True. However the label check runs in like 2s or so so I wouldn't count that as an issue 🤷

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 a pull request may close this issue.

4 participants