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: Add support to use threads instead of comments #1331

Merged
merged 1 commit into from
Nov 12, 2022
Merged

GitLab: Add support to use threads instead of comments #1331

merged 1 commit into from
Nov 12, 2022

Conversation

uncaught
Copy link
Contributor

@uncaught uncaught commented Oct 28, 2022

Implementation

  • Threads are used if an environment variable DANGER_GITLAB_USE_THREADS is set to a value of 1 or true.
  • Threads are completely removed with all answers (even of other users) if danger removes its main comment.

GitLab API change

  • A new method getMergeRequestDiscussions fetches all the threads (discussions) of a merge request
  • BREAKING The method createMergeRequestDiscussion has had its second argument changed from being the position object to be the actual options object for that gitbreaker API call, which contains the position.

Migration considerations

  • You can switch to using threads and back any time.
  • Only newly created comments are affected by the current setting.
  • Because existing comments can be turned into a thread by simply replying to them, danger will always delete its entire threads, no matter the setting.

@orta
Copy link
Member

orta commented Nov 1, 2022

Hey, this looks good to me from a read over it - I'm quite wary of this change being so substantial and only really having one test to ensure it doesn't break in the future. So, I'll give it a week to settle, if you can it'd be good to get some tests - if not, that's fine.

@uncaught
Copy link
Contributor Author

uncaught commented Nov 6, 2022

Thanks, @orta, I've added some tests - there were none at all before for the GitLab.ts.

I've also changed one key detail: danger will always delete its entire threads now, no matter the setting. Because I've noticed that you can simply turn danger's comments into threads by replying to them as another user. And that would leave the replies dangling if danger was then to remove its original comment.

GitLab's discussions API doesn't distinguish between comments and threads. You will always get all comments wrapped in a discussion object from that API.

@orta
Copy link
Member

orta commented Nov 12, 2022

Great, this looks good to me - I've given it a once over and will give it a second look and then merge soon!

Copy link
Member

@orta orta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@orta orta merged commit c3641dc into danger:main Nov 12, 2022
@orta
Copy link
Member

orta commented Nov 12, 2022

Thanks for the substantial PR! This has shipped as danger 11.2.0

@uncaught uncaught deleted the issue_1138_gitlab_threads branch November 14, 2022 06:56
@Jimimaku
Copy link

Jimimaku commented Nov 22, 2022

Implementation

  • Threads are used if an environment variable DANGER_GITLAB_USE_THREADS is set to a value of 1 or true.
  • Threads are completely removed with all answers (even of other users) if danger removes its main comment.

GitLab API change

  • A new method getMergeRequestDiscussions fetches all the threads (discussions) of a merge request
  • BREAKING The method createMergeRequestDiscussion has had its second argument changed from being the position object to be the actual options object for that gitbreaker API call, which contains the position.

Migration considerations

  • You can switch to using threads and back any time.
  • Only newly created comments are affected by the current setting.
  • Because existing comments can be turned into a thread by simply replying to them, danger will always delete its entire threads, no matter the setting.

@squarefrog
Copy link
Contributor

This PR has broken thread reporting for us, using GitLab 15.3. Using danger 11.1.4 we still get both inline and global warnings.

Using danger 11.2.0 or later, we only get global warnings, as they replace the body of the inline warnings. Full context available in #1351

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

Successfully merging this pull request may close these issues.

4 participants