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

Add thread support for GitLab #1140

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions source/commands/ci/runner.ts
Expand Up @@ -71,6 +71,7 @@ export const runRunner = async (app: SharedCLI, config?: Partial<RunnerConfig>)
ignoreOutOfDiffComments: app.ignoreOutOfDiffComments,
newComment: app.newComment || false,
removePreviousComments: app.removePreviousComments || false,
useThread: app.useThread || false
}

const processName = (app.process && addSubprocessCallAguments(app.process.split(" "))) || undefined
Expand Down
3 changes: 3 additions & 0 deletions source/commands/utils/sharedDangerfileArgs.ts
Expand Up @@ -33,6 +33,8 @@ export interface SharedCLI extends program.CommanderStatic {
newComment?: boolean
/** Removes all previous comment and create a new one in the end of the list */
removePreviousComments?: boolean
/** Use thread instance of notes */
useThread?: boolean
}

export default (command: any) =>
Expand All @@ -51,6 +53,7 @@ export default (command: any) =>
.option("--use-github-checks", "Use GitHub Checks", false)
.option("--ignoreOutOfDiffComments", "Ignore inline-comments that are in lines which were not changed", false)
.option("--newComment", "Makes Danger post a new comment instead of editing its previous one", false)
.option("--useThread", "Makes Danger post a thread instead of note", false)
.option(
"--removePreviousComments",
"Removes all previous comment and create a new one in the end of the list",
Expand Down
20 changes: 20 additions & 0 deletions source/dsl/GitLabDSL.ts
Expand Up @@ -292,3 +292,23 @@ export interface GitLabApproval {
}[]
| GitLabUser[]
}

export interface GitLabDiscussionAuthor {
id: number
name: string
username: string
state: boolean
}

export interface GitLabDiscussionNote {
id: number
type: string | null
body: string
author: GitLabDiscussionAuthor
}

export interface GitLabDiscussion {
id: number,
individual_note: boolean,
notes: GitLabDiscussionNote[]
}
8 changes: 8 additions & 0 deletions source/platforms/BitBucketCloud.ts
Expand Up @@ -67,6 +67,14 @@ export class BitBucketCloud implements Platform {
return true
}

supportsThreads() {
Copy link
Contributor

Choose a reason for hiding this comment

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

the type is infered?

return false;
}

updateOrCreateThread = async (): Promise<undefined> => {
return undefined;
}

/**
* Returns the response for the new comment
*
Expand Down
8 changes: 8 additions & 0 deletions source/platforms/BitBucketServer.ts
Expand Up @@ -117,6 +117,14 @@ export class BitBucketServer implements Platform {
return true
}

supportsThreads() {
return false;
}

updateOrCreateThread = async (): Promise<undefined> => {
return undefined;
}

/**
* Returns the response for the new comment
*
Expand Down
8 changes: 8 additions & 0 deletions source/platforms/FakePlatform.ts
Expand Up @@ -56,6 +56,14 @@ export class FakePlatform implements Platform {
return true
}

supportsThreads() {
return false;
}

updateOrCreateThread = async (): Promise<undefined> => {
return undefined;
}

async updateOrCreateComment(_dangerID: string, _newComment: string): Promise<string> {
return "https://github.com/orta/github-pages-with-jekyll/pull/5#issuecomment-383402256"
}
Expand Down
8 changes: 8 additions & 0 deletions source/platforms/GitHub.ts
Expand Up @@ -75,6 +75,14 @@ export function GitHub(api: GitHubAPI) {

getFileContents: api.fileContents,
executeRuntimeEnvironment,

supportsThreads() {
return false;
},

async updateOrCreateThread(): Promise<undefined> {
return undefined;
}
} as GitHubType
}

Expand Down
34 changes: 32 additions & 2 deletions source/platforms/GitLab.ts
Expand Up @@ -2,7 +2,7 @@ import GitLabAPI from "./gitlab/GitLabAPI"
import { Platform, Comment } from "./platform"
import { GitDSL, GitJSONDSL } from "../dsl/GitDSL"
import { GitCommit } from "../dsl/Commit"
import { GitLabDSL, GitLabJSONDSL, GitLabNote } from "../dsl/GitLabDSL"
import { GitLabDiscussion, GitLabDSL, GitLabJSONDSL, GitLabNote } from "../dsl/GitLabDSL"

import { debug } from "../debug"
import { dangerIDToString } from "../runner/templates/githubIssueTemplate"
Expand Down Expand Up @@ -92,6 +92,23 @@ class GitLab implements Platform {
return true
}

supportsThreads() {
return true;
}

updateOrCreateThread = async (dangerID: string, newComment: string): Promise<string> => {
const threads = await this.getDangerThreads(dangerID);

if (threads.length) {
await this.api.updateMergeRequestDiscussion(threads[0].id, threads[0].notes[0].id, newComment);
return `${this.api.mergeRequestURL}#note_${threads[0].notes[0].id}`;
} else {
const thread = await this.api.createMergeRequestDiscussion(newComment);

return `${this.api.mergeRequestURL}#note_${thread.notes[0].id}`;
Comment on lines +103 to +108
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the zero could be a const with a description. Looks like a magic number without any type of context of why the code should the zero index and not any other

}
}

updateOrCreateComment = async (dangerID: string, newComment: string): Promise<string> => {
d("updateOrCreateComment", { dangerID, newComment })

Expand Down Expand Up @@ -131,7 +148,7 @@ class GitLab implements Platform {

const mr = await this.api.getMergeRequestInfo()

return this.api.createMergeRequestDiscussion(comment, {
const thread = await this.api.createMergeRequestDiscussion(comment, {
position_type: "text",
base_sha: mr.diff_refs.base_sha,
start_sha: mr.diff_refs.start_sha,
Expand All @@ -141,6 +158,8 @@ class GitLab implements Platform {
new_path: path,
new_line: line,
})

return `${this.api.mergeRequestURL}#note_${thread.notes[0].id}`;
}

updateInlineComment = async (comment: string, id: string): Promise<GitLabNote> => {
Expand Down Expand Up @@ -178,6 +197,17 @@ class GitLab implements Platform {
)
}

getDangerThreads = async (dangerID: string) => {
const { id: dangerUserId } = await this.api.getUser()
const threads = await this.api.getMergeRequestThreads();
Comment on lines +201 to +202
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be resolved with Promise.all

Suggested change
const { id: dangerUserId } = await this.api.getUser()
const threads = await this.api.getMergeRequestThreads();
const [{ id: dangerUserId }, threads] = await Promise.all([this.api.getUser(), this.api.getMergeRequestThreads()])


return threads.filter(
(discussion: GitLabDiscussion) => {
return discussion.notes[0].author.id === dangerUserId && discussion.notes[0].body.includes(dangerIDToString(dangerID));
}
)
}

updateStatus = async (): Promise<boolean> => {
d("updateStatus", {})
return true
Expand Down
8 changes: 8 additions & 0 deletions source/platforms/LocalGit.ts
Expand Up @@ -71,6 +71,14 @@ export class LocalGit implements Platform {
return true
}

supportsThreads() {
return false;
}

updateOrCreateThread = async (): Promise<undefined> => {
return undefined;
}

async updateOrCreateComment(_dangerID: string, _newComment: string): Promise<string | undefined> {
return undefined
}
Expand Down
44 changes: 37 additions & 7 deletions source/platforms/gitlab/GitLabAPI.ts
@@ -1,17 +1,18 @@
import { RepoMetaData } from "../../dsl/BitBucketServerDSL"
import { api as fetch } from "../../api/fetch"
import {
GitLabApproval,
GitLabDiscussion,
GitLabDiscussionTextPosition,
GitLabInlineNote,
GitLabMR,
GitLabMRChange,
GitLabMRChanges,
GitLabMRCommit,
GitLabNote,
GitLabUserProfile,
GitLabRepositoryFile,
GitLabRepositoryCompare,
GitLabApproval,
GitLabRepositoryFile,
GitLabUserProfile,
} from "../../dsl/GitLabDSL"

import { Gitlab } from "gitlab"
Expand Down Expand Up @@ -136,7 +137,7 @@ class GitLabAPI {
return inlineNotes
}

createMergeRequestDiscussion = async (content: string, position: GitLabDiscussionTextPosition): Promise<string> => {
createMergeRequestDiscussion = async (content: string, position?: GitLabDiscussionTextPosition): Promise<GitLabDiscussion> => {
this.d(
"createMergeRequestDiscussion",
this.repoMetadata.repoSlug,
Expand All @@ -146,11 +147,15 @@ class GitLabAPI {
)
const api = this.api.MergeRequestDiscussions

const options: any = {};

if (position) {
options.position = position;
}

try {
// @ts-ignore
const result: string = await api.create(this.repoMetadata.repoSlug, this.repoMetadata.pullRequestID, content, {
position: position,
})
const result = await api.create(this.repoMetadata.repoSlug, this.repoMetadata.pullRequestID, content, options) as GitLabDiscussion;
this.d("createMergeRequestDiscussion", result)
return result
} catch (e) {
Expand All @@ -159,6 +164,26 @@ class GitLabAPI {
}
}

updateMergeRequestDiscussion = async (discussionId: number, noteId: number, body: string): Promise<GitLabDiscussion> => {
this.d(
"updateMergeRequestDiscussion",
this.repoMetadata.repoSlug,
this.repoMetadata.pullRequestID,
discussionId,
noteId,
body,
)

return await this.api.MergeRequestDiscussions.editNote(
this.repoMetadata.repoSlug,
this.repoMetadata.pullRequestID,
discussionId,
noteId,
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello Shym

I got a question, Why you need this ts-ignore here?

There is any cast issue with body property?

body
) as GitLabDiscussion;
}

createMergeRequestNote = async (body: string): Promise<GitLabNote> => {
this.d("createMergeRequestNote", this.repoMetadata.repoSlug, this.repoMetadata.pullRequestID, body)
const api = this.api.MergeRequestNotes
Expand Down Expand Up @@ -239,6 +264,11 @@ class GitLabAPI {
const compare = (await api.compare(projectId, base, head)) as GitLabRepositoryCompare
return compare.diffs
}
async getMergeRequestThreads(): Promise<GitLabDiscussion[]> {
this.d('getMergeRequestThreads');

return await this.api.MergeRequestDiscussions.all(this.repoMetadata.repoSlug, this.repoMetadata.pullRequestID) as GitLabDiscussion[];
}
}

export default GitLabAPI
6 changes: 5 additions & 1 deletion source/platforms/platform.ts
Expand Up @@ -69,6 +69,8 @@ export interface PlatformCommunicator {
supportsCommenting: () => boolean
/** Does the platform support inline comments? */
supportsInlineComments: () => boolean
/** Does the platform supports threads? */
supportsThreads: () => boolean
/** Allows the platform to do whatever it wants, instead of using the default commenting system */
handlePostingResults?: (results: DangerResults, options: ExecutorOptions) => void
/** Gets inline comments for current PR */
Expand All @@ -85,6 +87,8 @@ export interface PlatformCommunicator {
deleteMainComment: (dangerID: string) => Promise<boolean>
/** Replace the main Danger comment, returning the URL to the issue */
updateOrCreateComment: (dangerID: string, newComment: string) => Promise<string | undefined>
/** Replaces the created thread */
updateOrCreateThread: (dangerID: string, newComment: string) => Promise<string | undefined>
/** Sets the current PR's status */
updateStatus: (
passed: boolean | "pending",
Expand Down Expand Up @@ -148,7 +152,7 @@ export function getPlatformForEnv(env: Env, source: CISource): Platform {
// They need to set the token up for GitHub actions to work
if (env["GITHUB_EVENT_NAME"] && !ghToken) {
console.error(`You need to add GITHUB_TOKEN to your Danger action in the workflow:

- name: Danger JS
uses: danger/danger-js@X.Y.Z
${chalk.green(`env:
Expand Down
10 changes: 9 additions & 1 deletion source/runner/Executor.ts
Expand Up @@ -64,6 +64,8 @@ export interface ExecutorOptions {
newComment?: boolean
/** Removes all previous comment and create a new one in the end of the list */
removePreviousComments?: boolean
/** Uses threads instead of notes */
useThread?: boolean
}
// This is still badly named, maybe it really should just be runner?

Expand Down Expand Up @@ -314,7 +316,13 @@ export class Executor {
comment = githubResultsTemplate(dangerID, mergedResults, commitID)
}

if (this.options.newComment) {
if (this.options.useThread) {
if (!this.platform.supportsThreads) {
throw new Error(`Platform ${this.platform.name} does not support threads`);
}

issueURL = await this.platform.updateOrCreateThread(dangerID, comment);
} else if (this.options.newComment) {
issueURL = await this.platform.createComment(dangerID, comment)
} else {
issueURL = await this.platform.updateOrCreateComment(dangerID, comment)
Expand Down