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] Fixes to inline comments #1428

Merged
merged 5 commits into from
Apr 16, 2024
Merged

Conversation

heltoft
Copy link
Contributor

@heltoft heltoft commented Mar 6, 2024

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.

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.
@orta
Copy link
Member

orta commented Mar 14, 2024

( 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
Copy link
Member

orta commented Apr 16, 2024

OK, lets get this in and trigger a semver bump

@orta orta merged commit 3945263 into danger:main Apr 16, 2024
2 checks passed
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.

2 participants