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

Use GitLab project ID instead of project name #994

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/usage/gitlab.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ variable on your CI system:

- `DANGER_GITLAB_API_TOKEN` = An access token for the account which will post comments

You will need also to add an environment variable for your gitlab project id:
farabi marked this conversation as resolved.
Show resolved Hide resolved

- `DANGER_GITLAB_PROJECT_ID` = Gitlab project id can be found in (Settings -> General). if not defined, project name will be used instead

If you are using a GitLab version prior to 11.7 you will also need to define the following environment variable:

- `DANGER_GITLAB_HOST` = Defaults to `https://gitlab.com` but you can use it for your own url
Expand Down
2 changes: 1 addition & 1 deletion source/danger-incoming-process-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1189,7 +1189,7 @@
"type": "string"
},
"repoSlug": {
"description": "A path like \"artsy/eigen\"",
"description": "Project ID \"624\" or a path like \"artsy/eigen\"",
"type": "string"
}
},
Expand Down
12 changes: 6 additions & 6 deletions source/platforms/gitlab/_tests/_gitlab_api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { readFileSync } from "fs"
const nockBack = nock.back
nockBack.fixtures = __dirname + "/fixtures"

// We're testing https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/27117
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change dories me a bit, because this link used to work - but now it doesn't?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repo was moved to gitlab-foss. New MR link: https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/27117
I updated the tests and fixtures for all Gitlab tests in #1002

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think tests should run for both projectId and project path

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the link change should be reverted because https://gitlab.com/456/merge_requests/27117 is 404
or updated to the redirected link.

this is just a comment, the code can continue use numeric id

// We're testing https://gitlab.com/456/merge_requests/27117
// This has been chosen because it is already merged and publicly available, it's unlikely to change

/** Returns a fixture. */
Expand All @@ -27,7 +27,7 @@ describe("GitLab API", () => {

beforeEach(() => {
api = new GitLabAPI(
{ pullRequestID: "27117", repoSlug: "gitlab-org/gitlab-ce" },
{ pullRequestID: "27117", repoSlug: "456" },
getGitLabAPICredentialsFromEnv({
DANGER_GITLAB_HOST: "gitlab.com",
DANGER_GITLAB_API_TOKEN: "FAKE_DANGER_GITLAB_API_TOKEN",
Expand All @@ -37,22 +37,22 @@ describe("GitLab API", () => {

it("configures host from CI_API_V4_URL", () => {
api = new GitLabAPI(
{ pullRequestID: "27117", repoSlug: "gitlab-org/gitlab-ce" },
{ pullRequestID: "27117", repoSlug: "456" },
getGitLabAPICredentialsFromEnv({
CI_API_V4_URL: "https://testciapiv4url.com/api/v4",
DANGER_GITLAB_API_TOKEN: "FAKE_DANGER_GITLAB_API_TOKEN",
})
)

expect(api.projectURL).toBe("https://testciapiv4url.com/gitlab-org/gitlab-ce")
expect(api.projectURL).toBe("https://testciapiv4url.com/456")
})

it("projectURL is defined", () => {
expect(api.projectURL).toBe("https://gitlab.com/gitlab-org/gitlab-ce")
expect(api.projectURL).toBe("https://gitlab.com/456")
})

it("mergeRequestURL is defined", () => {
expect(api.mergeRequestURL).toBe("https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/27117")
expect(api.mergeRequestURL).toBe("https://gitlab.com/456/merge_requests/27117")
})

const sanitizeUserResponse = (nocks: NockDefinition[]): NockDefinition[] => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
"scope": "https://gitlab.com:443",
"method": "GET",
"path": "/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/changes",
"path": "/api/v4/projects/456/merge_requests/27117/changes",
"body": "",
"status": 200,
"response": {
Expand Down Expand Up @@ -58,7 +58,12 @@
],
"source_project_id": 13083,
"target_project_id": 13083,
"labels": ["Danger bot", "Plan", "backend", "backstage"],
"labels": [
"Danger bot",
"Plan",
"backend",
"backstage"
],
"work_in_progress": false,
"milestone": {
"id": 655280,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
"scope": "https://gitlab.com:443",
"method": "GET",
"path": "/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/commits",
"path": "/api/v4/projects/456/merge_requests/27117/commits",
"body": "",
"status": 200,
"response": [
Expand Down Expand Up @@ -51,7 +51,7 @@
"Etag",
"W/\"478bcb3a3b56ea7180c04159ff3adea5\"",
"Link",
"<https://gitlab.com/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/commits?id=gitlab-org%2Fgitlab-ce&merge_request_iid=27117&page=1&per_page=>; rel=\"first\", <https://gitlab.com/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/commits?id=gitlab-org%2Fgitlab-ce&merge_request_iid=27117&page=1&per_page=>; rel=\"last\"",
"<https://gitlab.com/api/v4/projects/456/merge_requests/27117/commits?id=456&merge_request_iid=27117&page=1&per_page=>; rel=\"first\", <https://gitlab.com/api/v4/projects/456/merge_requests/27117/commits?id=456&merge_request_iid=27117&page=1&per_page=>; rel=\"last\"",
"Vary",
"Origin",
"X-Content-Type-Options",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
"scope": "https://gitlab.com:443",
"method": "GET",
"path": "/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117",
"path": "/api/v4/projects/456/merge_requests/27117",
"body": "",
"status": 200,
"response": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
"scope": "https://gitlab.com:443",
"method": "GET",
"path": "/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/notes",
"path": "/api/v4/projects/456/merge_requests/27117/notes",
"body": "",
"status": 200,
"response": [
Expand Down Expand Up @@ -356,7 +356,7 @@
"Etag",
"W/\"edab8aad8eea37dd376785c34c7c250f\"",
"Link",
"<https://gitlab.com/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/notes?id=gitlab-org%2Fgitlab-ce&noteable_id=27117&order_by=created_at&page=1&per_page=20&sort=desc>; rel=\"first\", <https://gitlab.com/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/notes?id=gitlab-org%2Fgitlab-ce&noteable_id=27117&order_by=created_at&page=1&per_page=20&sort=desc>; rel=\"last\"",
"<https://gitlab.com/api/v4/projects/456/merge_requests/27117/notes?id=456&noteable_id=27117&order_by=created_at&page=1&per_page=20&sort=desc>; rel=\"first\", <https://gitlab.com/api/v4/projects/456/merge_requests/27117/notes?id=456&noteable_id=27117&order_by=created_at&page=1&per_page=20&sort=desc>; rel=\"last\"",
"Vary",
"Origin",
"X-Content-Type-Options",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
"scope": "https://gitlab.com:443",
"method": "GET",
"path": "/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/notes",
"path": "/api/v4/projects/456/merge_requests/27117/notes",
"body": "",
"status": 200,
"response": [
Expand Down Expand Up @@ -356,7 +356,7 @@
"Etag",
"W/\"edab8aad8eea37dd376785c34c7c250f\"",
"Link",
"<https://gitlab.com/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/notes?id=gitlab-org%2Fgitlab-ce&noteable_id=27117&order_by=created_at&page=1&per_page=20&sort=desc>; rel=\"first\", <https://gitlab.com/api/v4/projects/gitlab-org%2Fgitlab-ce/merge_requests/27117/notes?id=gitlab-org%2Fgitlab-ce&noteable_id=27117&order_by=created_at&page=1&per_page=20&sort=desc>; rel=\"last\"",
"<https://gitlab.com/api/v4/projects/456/merge_requests/27117/notes?id=456&noteable_id=27117&order_by=created_at&page=1&per_page=20&sort=desc>; rel=\"first\", <https://gitlab.com/api/v4/projects/456/merge_requests/27117/notes?id=456&noteable_id=27117&order_by=created_at&page=1&per_page=20&sort=desc>; rel=\"last\"",
"Vary",
"Origin",
"X-Content-Type-Options",
Expand Down
3 changes: 2 additions & 1 deletion source/platforms/platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,11 @@ export function getPlatformForEnv(env: Env, source: CISource): Platform {

// GitLab
if (env["DANGER_GITLAB_API_TOKEN"] || env["DANGER_PR_PLATFORM"] === GitLab.name) {
const repoSlug = env["DANGER_GITLAB_PROJECT_ID"] ? env["DANGER_GITLAB_PROJECT_ID"] : source.repoSlug
const api = new GitLabAPI(
{
pullRequestID: source.pullRequestID,
repoSlug: source.repoSlug,
repoSlug: repoSlug,
},
getGitLabAPICredentialsFromEnv(env)
)
Expand Down