Closed squarefrog closed 1 year ago
Looking into this further, I think I understand what is happening. I can see the note originally getting created:
2023-01-31T11:43:21.556Z danger:GitLabAPI createMergeRequestDiscussion {
id: 'c876fefd5f8ecb6993523da31743da1bc7efab87',
individual_note: false,
notes: [
{
id: 436868, <---------------------------- Take note of this ID
type: 'DiffNote',
body: '\n' +
'<!--\n' +
' 0 failure: \n' +
' 1 warning: inline warning, l...\n' +
' \n' +
' \n' +
' DangerID: danger-id-Danger;\n' +
' File: LinterTest.swift;\n' +
' Line: 22;\n' +
'-->\n' +
'\n' +
'- :warning: inline warning, line 22',
attachment: null,
author: [Object],
created_at: '2023-01-31T11:43:21.468Z',
updated_at: '2023-01-31T11:43:21.468Z',
system: false,
noteable_id: 41158,
noteable_type: 'MergeRequest',
commit_id: null,
position: [Object],
resolvable: true,
resolved: false,
resolved_by: null,
resolved_at: null,
confidential: false,
internal: false,
noteable_iid: 3,
commands_changes: {}
}
]
}
Then later on, this same note is updated with the global warnings, which replaces the body:
2023-01-31T11:43:22.048Z danger:GitLabAPI updateMergeRequestNote {
id: 436868, <--------------- We're updating the previously created ID, but with the global warnings
type: 'DiffNote',
body: '\n' +
'<!--\n' +
' 0 failure: \n' +
' 1 warning: global warning 2,...\n' +
' \n' +
' \n' +
' DangerID: danger-id-Danger;\n' +
'-->\n' +
'\n' +
'\n' +
'<table>\n' +
' <thead>\n' +
' <tr>\n' +
' <th width="50"></th>\n' +
' <th width="100%" data-danger-table="true">Warnings</th>\n' +
' </tr>\n' +
' </thead>\n' +
' <tbody><tr>\n' +
' <td>:warning:</td>\n' +
' <td>global warning 2, electic boogaloo</td>\n' +
' </tr>\n' +
' </tbody>\n' +
'</table>\n' +
'\n' +
'\n' +
'\n' +
'<p align="right">\n' +
' Generated by :no_entry_sign: <a href="https://danger.systems/js">dangerJS</a> against 15ae2210acd12aed65c2d087967c4af0762e8148\n' +
'</p>',
attachment: null,
author: {
id: 10,
username: 'bot',
name: 'Bot',
state: 'active',
avatar_url: 'https://secure.gravatar.com/avatar/4da90f9ec250a529f07e53ff28b6680b?s=80&d=identicon',
web_url: 'https://git.example.com/bot'
},
created_at: '2023-01-31T11:43:21.468Z',
updated_at: '2023-01-31T11:43:21.947Z',
system: false,
noteable_id: 41158,
noteable_type: 'MergeRequest',
commit_id: null,
position: {
base_sha: '08b1c527dbb2c31ee48444941c59b5fb22bfcd16',
start_sha: '08b1c527dbb2c31ee48444941c59b5fb22bfcd16',
head_sha: '15ae2210acd12aed65c2d087967c4af0762e8148',
old_path: 'LinterTest.swift',
new_path: 'LinterTest.swift',
position_type: 'text',
old_line: null,
new_line: 22,
line_range: null
},
resolvable: true,
resolved: false,
resolved_by: null,
resolved_at: null,
confidential: false,
internal: false,
noteable_iid: 3,
commands_changes: {}
}
Looking at the API code for GitLab, it seems like when deciding whether to create or update a discussion, we just pick the first discussion available:
updateOrCreateComment = async (dangerID: string, newComment: string): Promise<string> => {
d("updateOrCreateComment", { dangerID, newComment })
//Even when we don't set danger to create threads, we still need to delete them if someone answered to a single
// comment created by danger, resulting in a discussion/thread. Otherwise we are left with dangling comments
// that will most likely have no meaning out of context.
const discussions = await this.getDangerDiscussions(dangerID)
const firstDiscussion = discussions.shift()
const existingNote = firstDiscussion?.notes[0]
// Delete all notes from all other danger discussions (discussions cannot be deleted as a whole):
await this.deleteNotes(this.reduceNotesFromDiscussions(discussions)) //delete the rest
...
}
Is it possible that this is a naive approach?
I'm afraid I know very little about typescript or javascript, so I'm not sure how I'd go about investigating this further to create a PR.
Interestingly, if I create a new MR, with only the inline warning, I see it briefly in GitLab before it disappears completely. Looking at the pipeline log it seems like it's created successfully, then gets deleted at the end of the pipeline.
OK after significant investigation, I've found the issue. In GitLab.ts, we have the following:
updateOrCreateComment = async (dangerID: string, newComment: string): Promise<string> => {
d("updateOrCreateComment", { dangerID, newComment })
//Even when we don't set danger to create threads, we still need to delete them if someone answered to a single
// comment created by danger, resulting in a discussion/thread. Otherwise we are left with dangling comments
// that will most likely have no meaning out of context.
const discussions = await this.getDangerDiscussions(dangerID)
const firstDiscussion = discussions.shift()
const existingNote = firstDiscussion?.notes[0]
// Delete all notes from all other danger discussions (discussions cannot be deleted as a whole):
await this.deleteNotes(this.reduceNotesFromDiscussions(discussions)) //delete the rest
let newOrUpdatedNote: GitLabNote
if (existingNote) {
// update the existing comment
newOrUpdatedNote = await this.api.updateMergeRequestNote(existingNote.id, newComment)
} else {
// create a new comment
newOrUpdatedNote = await this.createComment(newComment)
}
// create URL from note
// "https://gitlab.com/group/project/merge_requests/154#note_132143425"
return `${this.api.mergeRequestURL}#note_${newOrUpdatedNote.id}`
}
The problem here is we update the existing inline note, with the global warnings. If I comment out the existing note check:
// if (existingNote) {
// update the existing comment
// newOrUpdatedNote = await this.api.updateMergeRequestNote(existingNote.id, newComment)
// } else {
// create a new comment
newOrUpdatedNote = await this.createComment(newComment)
// }
then run from my test repo:
yarn --cwd ../danger-js build; DEBUG=* node --inspect ../danger-js/distribution/commands/danger-ci.js
I can now see both the inline and global warnings
However, I'm unsure what side effects this has, as I'm unfamiliar with the code base and flow. Does anyone have any advice?
Reverting to danger js 11.1.4 fixes this issue for us.
I just came across this same bug today. I'll see if I can implement the fix you mention.
Another quick fix is to downgrade to 11.1.4 which is the last release before this bug was introduced.
Describe the bug I'm unable to get inline comments to show up correctly, and consistently, with danger-js and GitLab. While danger correctly adds a comment at the line I'd expect, the comment text is incorrect.
To Reproduce
.gitlab-ci.yml
file listed belowLinterTest.swift
listed belowLinterTest.swift
, removing the comments in lines 22-26.gitlab-ci.yml
```yaml stages: - analysis variables: LC_ALL: "en_US.UTF-8" LANG: "en_US.UTF-8" FF_ENABLE_JOB_CLEANUP: 'true' before_script: - eval "$(/opt/homebrew/bin/brew shellenv)" - brew install danger/tap/danger-js .mr: only: - merge_requests except: variables: - $CI_MERGE_REQUEST_TITLE=~ /^WIP:/ danger_ci: stage: analysis extends: .mr tags: - macos03 # adjust to your setup script: - echo "Running Danger" - DEBUG=* danger ci interruptible: true ```LinterTest.swift
```swift import Foundation class LinterTest { var a = "ab" func annoyLinter() { // Triggers swiftformat spaceAroundOperators for i in 0 ... 10 { print(i) } // touch file to check linting // Triggers swiftlint empty_count _ = a.count == 0 } enum State { case unknown case loading case finished } //enum NewState { //case unknown //case loading //case finished //} } ```dangerfile.ts
```typescript import { danger } from "danger"; warn("inline warning, line 16", "LinterTest.swift", 16); warn("global warning") warn("inline warning, line 22", "LinterTest.swift", 22); warn("global warning 2, electric boogaloo") ```Expected behavior There should be one inline comment, underneath line 22 of
LinterTest.swift
, with the textinline warning, line 22
. There should be 3 warnings on the main page of the merge request:inline warning, line 16
- as this line isn't part of the changed file, it may not show up at allglobal warning
global warning 2, electric boogaloo
Screenshots Overview
Changes tab
Your Environment