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

Gitlab FR: open a discussion rather than creating a note #1090

Open
brettdh opened this issue Dec 5, 2020 · 5 comments
Open

Gitlab FR: open a discussion rather than creating a note #1090

brettdh opened this issue Dec 5, 2020 · 5 comments

Comments

@brettdh
Copy link

brettdh commented Dec 5, 2020

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.

@orta
Copy link
Member

orta commented Dec 6, 2020

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: danger/danger#684 (comment)

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.

@brettdh
Copy link
Author

brettdh commented Dec 6, 2020

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.

@jeff-cook
Copy link

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

Thanks

@jeff-cook
Copy link

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.

@orta
Copy link
Member

orta commented Jan 1, 2021

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

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

No branches or pull requests

3 participants