danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.22k stars 363 forks source link

[Config Issue] GitHub Merge Queue - Danger server stuck in "waiting for status be reported" #1427

Open eppisapiafsl opened 4 months ago

eppisapiafsl commented 4 months ago

Describe the bug

We have configured "Danger" as a required step in the CI. It works as expected in Pull Request and After the merge process, but we face a problem configuring it for the new GitHub Merge Queue action (Configure to group 3 PRs at time)

To Reproduce Steps to reproduce the behavior:

  1. Configure 2 Danger workflow, one for Pull request and another one for Merge Group
    
    name: Danger JS 
    # The Merge Queue doesn't populate the PR information, neither the labels
    # We need a new DangerJS to pass the CI requirements
    on:
    merge_group:
        types:
            - checks_requested

jobs: danger_merge_group: runs-on: ubuntu-latest name: Danger JS steps: ...

name: Danger JS
on:
  merge_group:
  pull_request:
    types: ["opened", "edited", "reopened", "synchronize"]
env:
  # Github Access Token used to download Github distributed packages such as react-native-wrnch
  # See https://github.com/hinge-health/phoenix#setup-github-access-token
  GITHUB_ACCESS_TOKEN: ${{ secrets.SHARED_PACKAGES_READ }}
jobs:
  build:
    runs-on: ubuntu-latest
    name: Danger JS
    steps:
      ...
      - name: Danger
        run: yarn danger ci --id "Danger" --verbose
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

The danger file to run in Merge group is quite simple 😅

import { message } from "danger";

message('PR merged');
  1. Danger works as expected on Pull request but in the Merge group it get stuck (Even that the CI pass)
GHA pass Danger server stuck
Screenshot 2024-02-26 at 10 03 45 AM Screenshot 2024-02-26 at 10 03 31 AM

Expected behavior

Danger populates the result of the GHA in the Merge Group action.

Screenshots If applicable, add screenshots to help explain your problem.

Your Environment

software version
danger.js 11.3.1
node 20.9.0
npm 0.39.1
Operating System Ubuntu

Additional context

fbartho commented 4 months ago

First change I’d make is I would add the --failOnErrors parameter to your danger ci — from what I can see of your workflow, you expect GitHub Actions to report if Danger dies.

Your danger step might never have started to execute. An earlier step in the same job is still running, or some other (GitHub Actions) weirdness is happening.

@eppisapiafsl if you want more help, please share more of your GitHub workflow file. If that’s too complicated, I would dive in to GitHub Actions to watch the execution logs. You (or a repo-admin) can watch the full logs as each step in the workflow gets executed.

Watching on the PR-page can have some browser-weirdness / doesn’t show every event sequentially, it only shows the final result for the steps that are tagged as “required”. You want to prove that danger is even executing at all, and that may show log messages that help you debug the issue.

eppisapiafsl commented 4 months ago

Thanks @fbartho , I didn't share the whole file as the previous steps are just "yarn install" setups 😅

I can confirm, that the Danger JS GHA finished as expected but the webhook didn't post the message or get updated

Screenshot 2024-02-26 at 3 20 30 PM

I believe the merge_group trigger doesn't populate the PR information, so Danger gets stuck trying to find the PR to post 😅

fbartho commented 4 months ago

I believe the merge_group trigger doesn't populate the PR information

Ah! That’s probably it. In retrospect Merge Groups sort of include every PR that is in the group together, so maybe we’d want special handling so you could access the PR info for every PR involved?

If you read the GitHub Actions doc, there’s probably a schema/payload for the Merge Group event. That may be something that DangerJS could load in. — We have a danger.pr payload so maybe there should be an array of danger.mergeGroup?

I’m definitely motivated to support making merge groups more convenient, but the feature wasn’t generally available the last time I set a team up with a CI workflow.

eppisapiafsl commented 4 months ago

Yeah, I am still diving into the migration I also found other issues as the merge group trigger doesn't support all the options that we have in the pull_request

We have a danger.pr payload so maybe there should be an array of danger.mergeGroup This would be amazing, so we provide feedback when the CI fails in the merge group

In the short term, what about a parameter to tell the Danger server to "do nothing" danger --ci --merge-group

I could try with an empty danger file, but can be avoided with a parameter of if Danger recognizes the trigger