Danger: Incorrect MR ID for GitLab with Drone

[k0nserv]

Report

What did you do?

I ran Danger on Drone using a private GitLab instance.

What did you expect to happen?

I expected Danger to run correctly.

What happened instead?

Danger ran, but failed because the ID used to fetched the merge request details was the iid(the project specific merge request id) not the global merge request id that the API expects.

Your Environment

• Which CI are you running on? Drone
• Are you running the latest version of Danger? Yes

[k0nserv]

I am wondering how different CI systems will handle this. If there are multiple different approaches to this we might wanna support different approaches per MR. I wrote a small script that uses the Gitlab client from Dangers dependencies to get the correct ID and replace it in the environment.

[k0nserv]

require 'gitlab'

client = Gitlab.client(
  private_token: ENV['DANGER_GITLAB_API_TOKEN'],
  endpoint: ENV['DANGER_GITLAB_API_BASE_URL']
)

result = client.merge_requests(CGI.escape(ENV['DRONE_REPO']), iid: ENV['DRONE_PULL_REQUEST'].to_i, per_page: 1)

global_id = result[0].id
`DRONE_PULL_REQUEST=#{global_id} danger`

This is the workaround script

[orta]

Interesting, is this a bug with Drone, e.g. they should probably be providing that ENV var but right now they don't?

[k0nserv]

No they are providing the ENV var so it's not a bug with Drone. The problem is GitLab has two ids for merge requests. What they call iid which is the project specific merge request number starting at 1 for each project. There's also a global merge request id, id, that is used by the API. Hence making the request /api/v3/merge_requests/:iid will probably not return the expected merge reques. That is if you happen to even have access to that merge request.

[k0nserv]

@connorshea, @hjanuschka, @jk have you tried setting Danger up with GitLab using any CI? This will depend on what value the different CI systems export for the pull request id. If all use the project specific id(iid) we should use this workaround inside danger

[jk]

I've got GitLab + Jenkins running in a proof of concept state. So no real project yet. It works after working around some
minor issues.

Should I provide you the output of env from the build job? I can get that tomorrow when I'm back in the office.

[k0nserv]

Yeah if you could get me printenv from a merge request build that'd be nice :)

[connorshea]

Per the docs, you can also use api/v3/projects/:id/merge_requests?iid=42, although I assume that'd be limited by whether or not you have the project ID and whether or not the gitlab gem supports that endpoint.

We don't use Danger with GitLab currently because we only use GitLab CI for our projects, so unfortunately I don't have anything to test with.

[hjanuschka]

@k0nserv i actually fail to set it up. do you know if there is a sample .gitlab-ci.yml i can have a look at, i have like 5 projects where i could test it really well (and would need danger et-all :))

[k0nserv]

GitLab CI specifically isn't supported yet. There's work that has to happen from GitLab's side for that to be the case. There's a reference to it in the comments

[jk]

Yeah if you could get me printenv from a merge request build that'd be nice :)

@k0nserv Here you go, but take note that I do two workarounds in the buildjobs to stop danger from complaining:

export GIT_URL=GIT_URL_1
export LANG=en_US

Here is the (somewhat redacted) output from env:

RBENV_VERSION=2.2.0
BUILD_URL=https://build.host.tld/job/playground-danger-php-test/49/
M2=/opt/maven/current/bin
DANGER_GITLAB_API_BASE_URL=https://git.host.tld/api/v3
HUDSON_SERVER_COOKIE=e62815d80ee563ed
SHELL=/bin/bash
TERM=xterm-256color
BUILD_TAG=jenkins-playground-danger-php-test-49
QTDIR=/usr/lib64/qt-3.3
GIT_PREVIOUS_COMMIT=01fa2a1b738831064d09b5b82348fc3f4674c47d
QTINC=/usr/lib64/qt-3.3/include
gitlabTargetRepoHttpUrl=https://git.host.tld/playground/danger-php-test.git
[email protected]:playground/danger-php-test.git
ROOT_BUILD_CAUSE=SCMTRIGGER
WORKSPACE=/var/lib/jenkins/jobs/playground-danger-php-test/workspace
JOB_URL=https://build.host.tld/job/playground-danger-php-test/
USER=jenkins
RBENV_ROOT=/var/lib/jenkins/.rbenv
gitlabMergeRequestTitle=WIP: Something #trivial
[email protected]
gitlabTargetBranch=master
[email protected]:playground/danger-php-test.git
GIT_AUTHOR_NAME=Build Server
GIT_COMMITTER_NAME=Build Server
gitlabTargetNamespace=playground
NLSPATH=/usr/dt/lib/nls/msg/%L/%N.cat
GIT_COMMIT=ba5c4fa61a97ec1c7cbb8c53e9c8591a78344aa7
JENKINS_HOME=/var/lib/jenkins
ROOT_BUILD_CAUSE_SCMTRIGGER=true
PATH=[redacted]:/sbin:/usr/sbin:/bin:/usr/bin
BUILD_CAUSE_SCMTRIGGER=true
gitlabTargetRepoName=danger-php-test
[email protected]
PWD=/var/lib/jenkins/jobs/playground-danger-php-test/workspace
gitlabSourceNamespace=playground
HUDSON_URL=https://build.host.tld/
LANG=en_US   ← comes from export LANG=en_US
JOB_NAME=playground-danger-php-test
gitlabUserName=Firstname Lastname
gitlabSourceRepoHttpUrl=https://git.host.tld/playground/danger-php-test.git
BUILD_DISPLAY_NAME=#49
gitlabActionType=MERGE
gitlabSourceRepoName=danger-php-test
XFILESEARCHPATH=/usr/dt/app-defaults/%L/Dt
BUILD_CAUSE=SCMTRIGGER
BUILD_ID=49
JENKINS_URL=https://build.host.tld/
RBENV_SHELL=jenkins
JOB_BASE_NAME=playground-danger-php-test
GIT_PREVIOUS_SUCCESSFUL_COMMIT=01fa2a1b738831064d09b5b82348fc3f4674c47d
HOME=/var/lib/jenkins
SHLVL=3
M2_HOME=/opt/maven/current
gitlabSourceBranch=feature/danger4
gitlabSourceRepoHomepage=https://git.host.tld/playground/danger-php-test
GIT_BRANCH=danger-php-test/feature/danger4
JENKINS_SERVER_COOKIE=[redacted]
EXECUTOR_NUMBER=1
[email protected]:playground/danger-php-test.git   ← comes from export GIT_URL=GIT_URL_1
DANGER_GITLAB_HOST=git.host.tld
NODE_LABELS=master
LOGNAME=jenkins
gitlabMergeRequestIid=24
QTLIB=/usr/lib64/qt-3.3/lib
CVS_RSH=ssh
HUDSON_HOME=/var/lib/jenkins
NODE_NAME=master
LESSOPEN=|/usr/bin/lesspipe.sh %s
BUILD_NUMBER=49
[email protected]:playground/danger-php-test.git
[email protected]:playground/danger-php-test.git
HUDSON_COOKIE=34ca39c0-92c2-40de-a29f-00383b4b06db
DANGER_GITLAB_API_TOKEN=[redacted]
[email protected]
gitlabBranch=feature/danger4
gitlabMergeRequestId=351
G_BROKEN_FILENAMES=1
[email protected]:playground/danger-php-test.git
_=/bin/env

[k0nserv]

So for Jenkins no change should be required since we use the global ID. See here.

[k0nserv]

Has anyone tried other CIs with GitLab? @orta what are you thoughts around putting knowledge specific to the request source in CI implementations?

[orta]

Seems like the right answer to this, either that or ones that support gitlab can optionally have a mergeRequestID also?

[k0nserv]

Yeah actually probably makes more sense for the request source itself to have the logic to map from iid to id

[thebugcode]

I am also experiencing issues when setting this up. In particular danger tries to fetch the Merge Requests like this:
Request URI: https://gitlab.mydomain.net/api/v3/projects/myteam/myproject_name/merge_request/38823
(Gitlab::Error::NotFound)
And that indeed returns a 404. The right URI that returns valid data is https://gitlab.mydomain.net/api/v3/projects/1243/merge_requests/38823
so it looks like we should replace projects/myteam/my_project_name with projects/1234.

Any ideas why the endpoint seems to be different for me? I have GitLab Enterprise Edition 8.14.3

[orta]

try using a master version of the gitlab gem @thebugcode

[thebugcode]

@orta I fail to install the gitlab gem from master. Do you mind giving me a short instruction of how to install it from master such that danger can still use it?

[orta]

You'll need to use a Gemfile to set it up - I don't run the gem, so I can't really offer advice on why it might not work.

[k0nserv]

@thebugcode As long as the project path is urlencoded that should work. Both the numeric id and the project path is supported as the project id at least from my testing

[thebugcode]

Opening the URL with the project path fails even in my browser. It seems our Gitlab doesn't support this api request for some weird reason.
Given your knowledge of the source code, is there a way for me to force that the MR is fetched using ProjectID instead of project path? Maybe a environment variable gets checked

[k0nserv]

Nope not really. It's hardcoded to use the repo slug see here https://github.com/danger/danger/blob/master/lib/danger/request_sources/gitlab.rb#L95-L96

This should work though, I'll have a look at it tomorrow and see what could be up

[thebugcode]

@k0nserv Thanks. I'll keep trying to see why this doesn't work but according to my best guess right now: Fetching Gitlab MRs using the project path is not documented and not supported. The only documentation I can find is using the project ID: https://docs.gitlab.com/ee/api/merge_requests.html#get-single-mr

[k0nserv]

Yeah other docs don't explicitly reference this fact, but it's mentioned in the docs for /projects/:id. See here I believe that's still supposed to apply for other paths that start with /projects/:idand I did work at one point in any case

[k0nserv]

Just verified with our EE instance. /api/v3/projects/:escaped_repo_slug/merge_requests/:id definitely works

We're on version 8.17.3, but I believe we were probably on something like 8.14 when I last tried this

[thebugcode]

But does this work?
api/v3/projects/your_team/your_projectname/merge_request/:id
This is what danger is trying to access and fails

[k0nserv]

Hmm yeah that's I didn't see any code around escaping the slug. I could've sworn I implemented escaping the slug, but it seems to not be there anymore

[k0nserv]

Did some more digging. The escape was removed as it was added in the gitlab gem. See this commit. Danger doesn't include the gem anymore, but you can use danger-gitlab to get it. However that gem specifies ~> 3.7 for the gitlab gem. @orta Shouldn't that be change to depend on master or at least locked to the latest commit on master?

[orta]

We're waiting on the release ( see https://github.com/NARKOZ/gitlab/issues/270 https://github.com/NARKOZ/gitlab/pull/253#issuecomment-287523101 ) but I think Rubygems is like CocoaPods in that you can only depend on a released build

[orta]

so when 4.0 comes out, I'll update danger-gitlab to ~> 4.0

[k0nserv]

Right. In the meantime I think this might work

source 'https://rubygems.org'

gem 'danger-gitlab'
# `81aac2742aff0eb2464f9b562d0120cc9205b734` is the latest commit on
# NARKOZ/gitlab master.
gem 'gitlab', github: 'NARKOZ/gitlab', ref: '81aac2742aff0eb2464f9b562d0120cc9205b734' 

[thebugcode]

Awesome, thanks guys. This is what I was looking for, ill try it asap.

[orta]

I've updated danger-gitlab to 4.3.1 ( to coincide with when we introduced these changes ) which relies on gitlab 4.0.0. - thanks @asedge

[thebugcode]

Guys this is awesome . I'm dropping the workaround and coming back to the official version. Many thanks

[k0nserv]

@connorshea I'm confused about the docs for the API. Specifically https://docs.gitlab.com/ee/api/merge_requests.html#get-single-mr which specifies merge_request_iid which does not work per this issue. It should be merge_request_id I believe?

[connorshea]

@k0nserv with the v3 API we had both id and iid which were per-project or instance-wide IDs, now with v4 I believe we only use per-project IDs.

See https://docs.gitlab.com/ce/api/v3_to_v4.html

[k0nserv]

🎉 That's really great for my use case since Drone outputs the iid and I currently require custom logic to convert from it to the global id. Will the hooks that CIs use change to also recieve the iid rather than the global id?

[connorshea]

@k0nserv I assume Drone is currently using the v3 API to integrate with us, if they upgrade to v4 they'll have to switch over to using per-project :)

[k0nserv]

They're extracting the id from the ref eg refs/merge-requests/3/head in the version we're using(0.4). Which is why I need the code that translates from the iid(3) to the global id.

I just tried with GET /api/v4/projects/:id/merge_requests/3 against our instance running 8.17, but that still behaves the same for me ie 404s

[connorshea]

@k0nserv 8.17 only has v3 of the API, you'll need 9.0+ for v4

[k0nserv]

Ah okay I misunderstood the docs then :)

[k0nserv]

@connorshea Are you sure about that? On our instance I can use the v4 API, but the specific change relating to merge request ids does not seem to implemented

[connorshea]

@k0nserv we may have implemented a basic version of the v4 API in 8.17, I think? But the rest of the changes were made in 9.0.

[k0nserv]

Right, that makes sense then. We'll make sure to upgrade then :)