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

[BUG] Can't skip Found no issues or messages from Danger. Removing any existing messages on Gitlab. #1330

Closed
ivankatliarchuk opened this issue Oct 21, 2022 · 4 comments
Labels

Comments

@ivankatliarchuk
Copy link
Contributor

ivankatliarchuk commented Oct 21, 2022

Describe the bug

Current setup
We are running code reviews at a scale. Single repository with Danger job running. The job is triggered through a webhook on merge requests create/update and etc across the whole organisation.
Hope the diagrams express/explain it clearly.
To speed up our review, and do not waist time if review was already given, our danger file looks like this

const { danger, message, warn, fail, markdow } = require('danger');

if (isNotToSkipReview()) {
  ... danger magic ...
}

skipReview function does quick checks, they not relevant to a current problem. We are running code reviews at a scale. Basically there are rules to decide where or not to run a code review check

The issue is here

if (!hasMessages || this.options.removePreviousComments) {
  if (!hasMessages) {
    this.log(`Found no issues or messages from Danger. Removing any existing messages on ${this.platform.name}.`)
  } else {
    this.log(`'removePreviousComments' option specified. Removing any existing messages on ${this.platform.name}.`)
  }
  await this.platform.deleteMainComment(dangerID)
  const previousComments = await this.platform.getInlineComments(dangerID)
  for (const comment of previousComments) {
    if (comment && comment.ownedByDanger) {
      await this.deleteInlineComment(comment)
    }
  }
}

Basically when we skip a review, there are no messages, and previous comment is deleted.
Found no issues or messages from Danger. Removing any existing messages on Gitlab.

I understand a behaviour. Would it be possible to add an extra flag e.g. skipCommentDeleetion, reuseCurrentComment or something along this lines?

How its currently solved

Two jobs running. First check whether or not we should skip a review, and next job commence a review.
Would be nice to have a way to skip a removal too, probably even from within a danger at runtime

To Reproduce

Can provide some information on requeset. Gitlab and Gitlab CI access is required.

Expected behavior

Command danger ci does not remove previous Danger comments if there are no issues or messages.

Screenshots

Architecture
Automate Code Review

Your Environment

software version
danger.js 11.1.4
node 18
npm 8.19.1
Operating System Alpine(Docker)

Additional context

If there is a benefit, happy to create a documentation on hove to bake Danger+Gitlab+WebHooks at scale when issue resolved

@ivankatliarchuk ivankatliarchuk changed the title [BUG] [BUG] Can't skip Found no issues or messages from Danger. Removing any existing messages on Gitlab. Oct 21, 2022
@ivankatliarchuk ivankatliarchuk changed the title [BUG] Can't skip Found no issues or messages from Danger. Removing any existing messages on Gitlab. [BUG] Can't skip Found no issues or messages from Danger. Removing any existing messages on Gitlab. Oct 21, 2022
@orta
Copy link
Member

orta commented Oct 21, 2022

Looks like you're re-creating Peril but have you considered setting a unique danger id per run? I'd imagine it would have the effect you want

@ivankatliarchuk
Copy link
Contributor Author

Probably something similar to Peril, but for Gitlab. I'll give it a try thanks.

@ivankatliarchuk
Copy link
Contributor Author

I tested it, its solves a problem, it kind of does, but creates a new one. As of now, no deletion of comments happens, but it creates every time a new Danger comment and there is no way to keep single comment

@orta
Copy link
Member

orta commented Oct 24, 2022

Fair, I think I'd be OK with this as an environment variable and tests to make sure it doesn't break - it's such a niche case that I don't think anyone else has any use for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants