danger / danger-js

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

[BUG] `parent_ids` of GitLab MR commits is always an empty array #1418

Open frantic1048 opened 7 months ago

frantic1048 commented 7 months ago

Describe the bug

for (const commit of danger.gitlab.commits) {
  commit.parent_ids; // <- this is always an empty array
}

To Reproduce

This MR compares results from danger's parent_ids and git CLI(the expected result).

https://gitlab.com/frantic1048/danger-empty-parent-ids/-/merge_requests/1

Expected behavior

A commit's parent_ids should contain all parent ids.

Your Environment

software version
danger.js 11.3.1
node 20.10.0
pnpm 8.12.0
Operating System node@sha256:445acd9b2ef7e9de665424053bf95652e0b8995ef36500557d48faf29300170a

Additional context

This is actually an upstream issue of GitLab: Commit parent IDs list is empty in some REST API's.

To ensure a smoother experience, we may implement a workaround in danger.

fbartho commented 7 months ago

@frantic1048 — I’m not sure DangerJS can fix this right? Since the upstream issue is 4 years old?…

What would you like to do here?

frantic1048 commented 7 months ago

@fbartho Here are 2 ways I found to workaround it, but both of them have their own drawbacks. I'm currently using the second workaround to perform checks around parent_ids.

  1. Use GitLab Commits API

    Retrieve parent_ids via GET /projects/:id/repository/commits/:sha, since this API is not suffering from the same issue. However, using this API may lead to lots of API calls when there are many commits in the merge request.

    e.g. https://gitlab.com/api/v4/projects/53011396/repository/commits/f1c6ae6e

  2. Use git CLI

    Use git show -s --pretty=format:%p <commit> to retrieve parent_ids. This more efficient than the first workaround, but it requires a local git repository.

    To ensure essential git history is available, we may need to run git fetch $CI_COMMIT_REF_NAME first.

    e.g. https://gitlab.com/frantic1048/danger-empty-parent-ids/-/blob/f1c6ae6e52db3a939c64c89f19227b16356da6ef/dangerfile.ts#L11-15

Because of the drawbacks, I'm not promoting any of the workarounds into danger-js. Just sharing my findings here in case anyone else is facing the same issue.

For danger-js, I think it's better to add some warning messages(through comments on the type definition) about parent_ids field in the merge request payload, so that users can be aware of the issue.