[BUG] GitLab - Large PRs have incomplete created_files, modified_files and deleted_files

[uncaught]

Describe the bug The files listed under danger.git.*_files are incomplete - at least in GitLab. It seems only a limited amount of files are listed and possibly only the same number of files that GitLab will show in the diff view of the merge request.

GitLab shows 898+ files, danger-js only gives me 898 files, while a git diff --name-status origin/master...origin/my_branch|wc -l shows 2549 files.

This leads to a crucial pre-merge test not to be executed for many files that are actually contained.

To Reproduce Steps to reproduce the behavior:

1. Have a large merge request with 900+ files in GitLab.
2. Check danger.git.created_files or danger.git.modified_files or danger.git.deleted_files to contain all the files.

Expected behavior All files in the merge request are contained in danger.git.

Screenshots

This is what GitLab shows:

These are the counts of the danger.git API that I've printed out (0 added, 2 modified, 896 deleted):

And this is the expected count of files included in the merge request (3 added, 10 modified, 2528 deleted):

Your Environment

software	version
danger.js	11.0.2 (noticed with 10.6.6 then upgraded)
node	14.17.5
yarn	1.22.5
GitLab Enterprise	14.6.2
Operating System	alpine3.14

Additional context It is curious that the "898+" GitLab is showing matches so exactly to the "898" files that danger is giving me. Is there some pagination in the GitLab API that you are using and not pulling additional pages?

[orta]

Makes sense, the current implementation uses:

https://github.com/danger/danger-js/blob/1822fe124fbbd1d78ac43f3ee875685ef2b04392/source/platforms/gitlab/GitLabAPI.ts#L116-L125

Which doesn't look like it is feasible to paginate: https://docs.gitlab.com/ee/api/merge_requests.html#get-single-mr-changes

Perhaps Danger should provide a warning when the number or changes are bigger than files changed?

[uncaught]

Well, if danger is supposed to abstract the git layer here, it should behave indiscriminate of platforms. So if for github all files are included, then for gitlab the same should apply. Then the call should be made with the mentioned access_raw_diffs, even if that takes longer to load.

Because the gitlab-ci runs on a regular cloned repository, would it not be faster to use git diff in danger instead of calling the gitlab API for changes?

[fbartho]

@uncaught the DangerJS GitLab Provider is basically a plugin abstracting the operations that GitLab supports.

The internals of a provider are “private” to that provider, so if the GitLab-supporting-devs decide to use different API calls, or use appropriate CLI commands instead of using API calls, that’s totally a fair choice to balance between maintainability, consistency, and performance.

The goals are “correct”, followed by “fast enough” balanced with “easy enough to maintain”.

Remember:

• performance is often a non-obvious battle, if you’re making a performance change, you need to measure & justify the speed + memory + code tradeoffs
• Shelling out to a CLI command incurs the cost of ensuring that the CLI command 1. exists 2. Is at the right version 3. Has the right config 4. Has the right access to the project history
• Shelling out to any CLI commands also runs a security risk — user untrusted paths, data, potentially hostile actors, OS differences, etc

[uncaught]

@fbartho, so if the GitLab Provider is private, is this not the right repository for this issue? Where should I go with it to reach those GitLab-supporting-devs?

[fbartho]

Sorry for the lagged response @uncaught -- I think I forgot to hit send.

DangerJS is an open-source community, with many many contributors. This indeed is the right place to reach those GitLab-supporting-devs, however I don't know that we have a "bat-signal" to summon the right people to ask.

You too can contribute here, and I didn't mean that the "GitLab Provider is private" exactly, I meant that for many consumers of DangerJS, we don't look at the internals of how other providers implement their code. You could rewrite the GitLab provider and submit a PR to DangerJS, and we would likely approve it as long as it follows normal coding best practices, and other GitLab users give it a thumbs up.

As long as you're not breaking other providers, or changing the common-core APIs, then the only people you'd need to convince are other DangerJS+GitLab users.