[GitLab] Fixes to inline comments #1428
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The currently used node environment has reached end of life. Updating to 18.x which is the version with Maintenance status for Node.js. - Upgraded node engine to >= 18 - Upgraded @types/node to 18.19.18
Since we are now using node engine 18 we will need to bump some dependencies to be able to build and run tests properly.
Major version 36 of gitbeaker contains changes to the type system and the structure of the framework. Moved to gitbeaker/rest which is the expected package to consume going forward, and adjusted our usage of the types to match the changes in the framework. Additionally gitbeaker now leverages native-fetch under the hood which nock does not support. Version 4.0.0 of nock in beta can handle testing native-fetch, but does not yet support recording fixtures. To continue using nock for testing without updating to the beta-version, a fetch polyfill has been introduced to use for the gitlab api tests.
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 inlinePositionParser 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.
Removing system resolved danger discussions on old versions of the diff can result in a state where the system note becomes the first note on the discussion resulting in poor change history on the MR. To remedy this we 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.
|
( 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) |
|
OK, lets get this in and trigger a semver bump |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.