danger / danger-js

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

[BUG] Danger.js comments removed from Github PR after merge #1402

Closed fabiendem closed 1 year ago

fabiendem commented 1 year ago

Describe the bug

We noticed that when a PR receives danger.js messages or warnings, after the merge, the Danger.JS comment grouping those is deleted from the PR. Even if we scroll back in the history of comments on the PR, we can no longer see Danger.js messages.

This is causing issues sometimes when we're discussing on PRs which turn out to be problematic. We would like to point the developer to a Danger.js comment ("we told you so"), but the comment is no longer present.

To Reproduce Steps to reproduce the behavior:

  1. Create a PR
  2. Get Danger.JS messages on it
  3. Merge the PR
  4. Danger.JS comments are no longer present

Expected behavior

Danger.JS comments are still present after the merge of the PR.

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

N/A

Your Environment

software version
danger.js 11.2.1
node 18.15.0
yarn 1.22.19
Operating System Ubuntu on CI

Additional context

Are we missing some parameter to enable this behavior?

Cheers!

orta commented 1 year ago

You probably want to use --newComment to always post a new comment instead of having Danger continually editing the existing message, this IMO is a bit spammy, but it is the only way in which you get the full history

But danger will only delete the comment if it was ran on the CI when the CI run had no issues, so I'm not sure how you would get a situation where a PR after a merge is running danger but that's likely to do with your CI infra

fabiendem commented 1 year ago

Thank you, yeah --newComment is too spammy.

But danger will only delete the comment if it was ran on the CI when the CI run had no issues

We have some checks which will print simple messages and warnings, so which don't mark the PR checks as "failed".

But danger will only delete the comment if it was ran on the CI when the CI run had no issues, so I'm not sure how you would get a situation where a PR after a merge is running danger but that's likely to do with your CI infra

💡 🚀 I think you just answered my question. On our repo, we have two sets of Dangers checks, one running while the PR is open and changing, and one running after the merge. The checks running after the merge are probably deleting the previous messages, because the set of checks is different. I think we need to make sure the entire set of danger checks runs after the merge as well.

If you're curious, via Github Actions, we run Danger on a PR after a merge using the following code:

name: Danger JS after merge
on:
  pull_request_target:
    types:
      - closed
env:
  # ......
jobs:
  if_merged:
    if: github.event.pull_request.merged == true
    runs-on: ubuntu-latest
    steps:
      -   # ...... steps for setup
      # ....
      - name: Danger after merge
        run: yarn danger ci --dangerfile "./dangerfile_after_merge.ts"
fabiendem commented 1 year ago

My colleague @eppisapiafsl suggested that we could use --newComment just for the "Danger after merge" run, so we have two sets of comments, and this one runs once anyway. We will try this. If it doesn't work, we will run "the entire set of danger checks after the merge as well."

orta commented 1 year ago

You may want to use --danger-id to differentiate them, either way, this isn't really a bug nor feature request - so I'm gonna close it up

fabiendem commented 1 year ago

Makes sense, thank you!