danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.26k stars 368 forks source link

fix: GitLab Inline comments deleted with main comment updates #1382

Closed DavidBrunow closed 1 year ago

DavidBrunow commented 1 year ago

Fixes #1351 and possibly #1380.

I verified this fix with an inline comment and a main Danger note with DANGER_GITLAB_USE_THREADS both set and unset. With it unset all behavior looked good to me.

With that flag set, I am seeing an issue where the main Danger note was replacing the inline comment. Since that is not the intended behavior I tested the latest release with that flag set and also saw the same bad behavior there. Since that behavior is consistent I'd like to move forward with this pull request without trying to fix that behavior.

DavidBrunow commented 1 year ago

@squarefrog No problem if you don't have the bandwidth, but if you do I'd appreciate you giving this branch a test.

squarefrog commented 1 year ago

I’m off work until next week but I can give it a try then, thanks for the tag!

squarefrog commented 1 year ago

I'm really sorry for the delay in testing this. I've been on leave from work, then we've had issues with our CI setup. I'll need to see if I can figure out how to build the binary of this branch then try it.

DavidBrunow commented 1 year ago

I'm really sorry for the delay in testing this. I've been on leave from work, then we've had issues with our CI setup. I'll need to see if I can figure out how to build the binary of this branch then try it.

Not a problem, life is busy and I keep getting distracted from this as well :)

squarefrog commented 1 year ago

The good news is I figured out how to create a build of this and put it on our CI and it seems to work really well. Great job with this @DavidBrunow 🙌

DavidBrunow commented 1 year ago

@orta Do you want to review or do you recommend any other reviewers before merging this?

LucasWeber-git commented 1 year ago

We are facing the same problem here. @orta it would be great if you could approve this merge.

orta commented 1 year ago

Yeah, is @squarefrog is happy, I am happy

orta commented 1 year ago

Shipped