danger-js icon indicating copy to clipboard operation
danger-js copied to clipboard

Gitlab FR: open a discussion rather than creating a note

Open brettdh opened this issue 3 years ago • 5 comments

In Gitlab merge requests, there are two types of comments:

  • Note: Appears in the merge request timeline but does not require a response before merging
  • Discussion: Appears as the start of a resolvable thread; requires clicking "Resolve" before merging

For Danger, a discussion is preferable IMO, since it enables the repo administrator to require that Danger's notes be marked as resolved before the code is merged, thereby increasing the likelihood that the merge request author has addressed Danger's comments.

Glancing through the code briefly, I notice that Danger already uses Discussions for inline code comments, so I think it would be trivial to use it for "main" comments as well. If this seems reasonable, I can look at a PR sometime soon.

brettdh avatar Dec 05 '20 19:12 brettdh

Assuming this is like reviews in GitHub, what we have done with that in Danger ruby is allow for a subset DSL for those capabilities: https://github.com/danger/danger/issues/684#issuecomment-269467745

Depending on what you can do with it, I'd look into using about a specific DSL for this e.g.

danger.gitlab.newDiscussion((d) => {
  d.message("x")
  d.fail("y")
})

I probably wouldn't be up for changing the default to something which requires clicking resolve by default (not this late into the game) but it's feasible if the PR is well tested to have a config to switch to use that instead somehow.

orta avatar Dec 06 '20 11:12 orta

Makes sense; thanks for the feedback. In both GitHub and Gitlab, discussions and reviews are different things, but I think I know what direction to go in now, enough to work on a PR.

brettdh avatar Dec 06 '20 16:12 brettdh

Getting started with Danger and this is defiantly something I'm looking for.

Thanks

jeff-cook avatar Dec 31 '20 22:12 jeff-cook

How would the example above work if importing additional Dangerfiles?

danger.gitlab.newDiscussion((d) => {
  d.message("x")
  d.fail("y")
})

I would think having a config option to create the whole thing as a discussion vs a note would be better, than having to use special commands in the Dangerfile.

jeff-cook avatar Dec 31 '20 22:12 jeff-cook

It could work by dependency injection, e.g. you pass the discussion as a param, I would do the same for message, error etc anyway

orta avatar Jan 01 '21 09:01 orta