danger / danger-js

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

[GitLab] Fixes to inline comments #1428

Closed heltoft closed 3 months ago

heltoft commented 4 months ago

The changes here aims to solve some issues that currently exists related to inline comments on GitLab. The commits in this pull request has been structured with as self-contained changes as possible along with a commit message describing the change in detail. As this is a big pull request the idea is that this should ease the burden for the reviewers. From here on is a description of the changes and what they aim to solve.

Moving to gitbeaker/rest #1386

Updating to a newer version of gitbeaker ultimately removes the deprecation warning from #1386 while solving the error outlined in #1412.

However version 36 of gitbeaker contains major changes to the type system and the structure of the framework, requiring some changes on our side to adjust the usage of the types and api to match the changes in the framework.

Additionally the newer versions of gitbeaker requires us to be on node engine >= 18. The currently used node environment has reached end of life and updating to 18.x as gitbeaker requires is from my point of view considered reasonable as that is the version with Maintenance status for Node.js.

Updating dependencies

A few dependencies is required to update to be able to build, run all our flows and test with the updated node engine.

Making GitLab inline comments show up correctly #1405

To be able to correctly add inline comments on GitLab we need to provide the file path pre & post change, along with the line number pre & post change.

Introduced an inline position parser that calculates these values by inspecting the structured diff. A binary search approach was used to make sure that the work to calculate the changes does not grow exponentially with the number of changes in the diff.

Improving MR history for inline comments on GitLab

Currently the inline comments posted by danger is deleted on each run which can result in poor change history on an MR if the comments was resolved by the system due to the change being fixed in a newer version of the diff.

To remedy this we now look for system notes on resolved danger discussions and abort deletion. This could be considered a brittle check, as there is no data to check to see if the system note did resolve it. However currently I am unaware of any other events triggering system notes on a discussion itself. As long as that assumption holds true the check should work as intended and not delete system resolved discussions.

orta commented 4 months ago

( I'm moving house ATM, but I think we should take this PR, I'll just need to make sure I have the time to figure out the node version breaking changes WRT Danger Swift/Rust et al)

orta commented 3 months ago

OK, lets get this in and trigger a semver bump