High priority queue refactoring & improvements (#67)

Author: pepakriz

.github/workflows/tests.yml

@@ -33,7 +33,7 @@ jobs:
 
       - name: Check Helm Chart
         id: lint
-        uses: helm/[email protected]alpha.3
+        uses: helm/[email protected]rc.1
         with:
           command: lint
 
@@ -45,6 +45,6 @@ jobs:
         if: steps.lint.outputs.changed == 'true'
 
       - name: Check Helm Install
-        uses: helm/[email protected]alpha.3
+        uses: helm/[email protected]rc.1
         with:
           command: install

README.md

@@ -23,7 +23,7 @@ Reasons can be for example:
 - Autogenerated commit message based on MR title (Suffixed with link to the original MR).
 - The blocking jobs are triggered automatically. Very useful for E2E testing etc.
 - Bot skips squashing when `bot:skip-squash` label is present on the MR.
-- Bot assigns MR to the beginning of the queue when `bot:hi-priority` is present. Useful for hotfixes etc.
+- Bot assigns MR to the beginning of the queue when `bot:high-priority` is present. Useful for hotfixes etc.
 
 ## Pre-Installation requirements
 
@@ -92,5 +92,5 @@ GITLAB_AUTH_TOKEN="<token>" yarn run start
 | `SQUASH_MERGE_REQUEST` | `true` | It'll squash commits on merge |
 | `AUTORUN_MANUAL_BLOCKING_JOBS` | `true` | It'll autorun manual blocking jobs before merge |
 | `SKIP_SQUASHING_LABEL` | `bot:skip-squash` | It'll skip squash when MR contains this label |
-| `HI_PRIORITY_LABEL` | `bot:hi-priority` | It'll put MR with this label to the beginning of the queue |
+| `HI_PRIORITY_LABEL` | `bot:high-priority` | It'll put MR with this label to the beginning of the queue |
 | `SENTRY_DSN` | `` | It'll enable Sentry monitoring |

charts/gitlab-merger-bot/Chart.yaml

@@ -2,7 +2,7 @@ apiVersion: v1
 appVersion: "1.0"
 description: A Helm chart for Kubernetes
 name: gitlab-merger-bot
-version: 1.2.3
+version: 1.2.4
 home: https://github.com/pepakriz/gitlab-merger-bot
 maintainers:
   - name: pepakriz

charts/gitlab-merger-bot/templates/deployment.yaml

@@ -55,8 +55,8 @@ spec:
           value: "{{ .Values.settings.sentryDsn }}"
         - name: SKIP_SQUASHING_LABEL
           value: "{{ .Values.settings.skipSquashingLabel }}"
-        - name: HI_PRIORITY_LABEL
-          value: "{{ .Values.settings.hiPriorityLabel }}"
+        - name: HIGH_PRIORITY_LABEL
+          value: "{{ .Values.settings.highPriorityLabel }}"
         resources:
 {{ toYaml .Values.resources | indent 10 }}
 {{- if .Values.nodeSelector }}

charts/gitlab-merger-bot/values.yaml

@@ -30,4 +30,4 @@ settings:
   squashMergeRequest: true
   autorunManualBlockingJobs: true
   skipSquashingLabel: ""
-  hiPriorityLabel: ""
+  highPriorityLabel: ""

src/MergeRequestAcceptor.ts

@@ -26,6 +26,7 @@ export enum AcceptMergeRequestResultKind {
 	WaitingForApprovals,
 	UnresolvedDiscussion,
 	Unauthorized,
+	InvalidSha,
 }
 
 interface Response {
@@ -92,6 +93,11 @@ interface UnauthorizedResponse extends Response {
 	mergeRequestInfo: MergeRequestInfo;
 }
 
+interface InvalidShaResponse extends Response {
+	kind: AcceptMergeRequestResultKind.InvalidSha;
+	mergeRequestInfo: MergeRequestInfo;
+}
+
 export type AcceptMergeRequestResult = SuccessResponse
 	| ClosedMergeRequestResponse
 	| ReassignedMergeRequestResponse
@@ -104,6 +110,11 @@ export type AcceptMergeRequestResult = SuccessResponse
 	| UnresolvedDiscussionResponse
 	| UnauthorizedResponse;
 
+export type MergeMergeRequestResult = SuccessResponse
+	| CanNotBeMergedResponse
+	| InvalidShaResponse
+	| UnauthorizedResponse;
+
 interface AcceptMergeRequestOptions {
 	ciInterval: number;
 	removeBranchAfterMerge: boolean;
@@ -140,23 +151,97 @@ export const filterBotLabels = (labels: string[]): string[] => {
 	return labels.filter((label) => !values.includes(label));
 };
 
-export const acceptMergeRequest = async (gitlabApi: GitlabApi, mergeRequest: MergeRequest, user: User, options: AcceptMergeRequestOptions): Promise<AcceptMergeRequestResult> => {
-	let mergeRequestInfo: MergeRequestInfo;
+export const acceptMergeRequest = async (
+	gitlabApi: GitlabApi,
+	mergeRequest: MergeRequest,
+	user: User,
+	options: AcceptMergeRequestOptions,
+): Promise<MergeMergeRequestResult> => {
+	console.log(`[MR][${mergeRequest.iid}] Calling merge request`);
+	const mergeRequestInfo = await gitlabApi.getMergeRequestInfo(mergeRequest.project_id, mergeRequest.iid);
+	const response = await gitlabApi.sendRawRequest(`/api/v4/projects/${mergeRequestInfo.project_id}/merge_requests/${mergeRequestInfo.iid}/merge`, RequestMethod.Put, {
+		should_remove_source_branch: options.removeBranchAfterMerge,
+		sha: mergeRequestInfo.diff_refs.head_sha,
+		squash: mergeRequestInfo.labels.includes(options.skipSquashingLabel) ? false : options.squashMergeRequest,
+		squash_commit_message: `${mergeRequestInfo.title} (!${mergeRequestInfo.iid})`,
+		merge_commit_message: `${mergeRequestInfo.title} (!${mergeRequestInfo.iid})`,
+	});
+
+	if (response.status === 405) {
+		console.log(`[MR][${mergeRequestInfo.iid}] 405 - cannot be merged`);
+		return {
+			kind: AcceptMergeRequestResultKind.CanNotBeMerged,
+			mergeRequestInfo,
+			user,
+		};
+	}
+
+	if (response.status === 406) {
+		console.log(`[MR][${mergeRequestInfo.iid}] 406 - already merged`);
+		return {
+			kind: AcceptMergeRequestResultKind.SuccessfullyMerged,
+			mergeRequestInfo,
+			user,
+		};
+	}
+
+	if (response.status === 409) {
+		console.log(`[MR][${mergeRequestInfo.iid}] 409 - SHA does not match HEAD of source branch`);
+		return {
+			kind: AcceptMergeRequestResultKind.InvalidSha,
+			mergeRequestInfo,
+			user,
+		};
+	}
+
+	if (response.status === 401) {
+		console.log(`[MR][${mergeRequestInfo.iid}] 409 - Unauthorized`);
+		return {
+			kind: AcceptMergeRequestResultKind.Unauthorized,
+			mergeRequestInfo,
+			user,
+		};
+	}
+
+	if (response.status !== 200) {
+		throw new Error(`Unsupported response status ${response.status}`);
+	}
+
+	const data = await response.json();
+	if (typeof data !== 'object' && data.id === undefined) {
+		console.error('response', data);
+		throw new Error('Invalid response');
+	}
+
+	if (!containsLabel(mergeRequestInfo.labels, BotLabels.Accepting)) {
+		await gitlabApi.updateMergeRequest(mergeRequestInfo.project_id, mergeRequestInfo.iid, {
+			labels: [...filterBotLabels(mergeRequestInfo.labels), BotLabels.Accepting].join(','),
+		});
+	}
+
+	return {
+		kind: AcceptMergeRequestResultKind.SuccessfullyMerged,
+		mergeRequestInfo,
+		user,
+	};
+};
+
+export const runAcceptingMergeRequest = async (gitlabApi: GitlabApi, mergeRequest: MergeRequest, user: User, options: AcceptMergeRequestOptions): Promise<AcceptMergeRequestResult> => {
 	let numberOfPipelineValidationRetries = defaultPipelineValidationRetries;
 	let numberOfRebasingRetries = defaultRebasingRetries;
 
 	while (true) {
-		const tasks: Promise<any>[] = [sleep(options.ciInterval)];
-		mergeRequestInfo = await gitlabApi.getMergeRequestInfo(mergeRequest.project_id, mergeRequest.iid);
-
-		if (!containsAssignedUser(mergeRequestInfo, user)) {
-			return {
-				kind: AcceptMergeRequestResultKind.ReassignedMergeRequest,
-				mergeRequestInfo,
-				user,
-			};
+		const mergeResponse = await acceptMergeRequest(gitlabApi, mergeRequest, user, options);
+		if (
+			mergeResponse.kind === AcceptMergeRequestResultKind.SuccessfullyMerged
+			|| mergeResponse.kind === AcceptMergeRequestResultKind.Unauthorized
+		) {
+			return mergeResponse;
 		}
 
+		const mergeRequestInfo = mergeResponse.mergeRequestInfo;
+		const tasks: Promise<any>[] = [sleep(options.ciInterval)];
+
 		if (mergeRequestInfo.state === MergeState.Merged) {
 			return {
 				kind: AcceptMergeRequestResultKind.SuccessfullyMerged,
@@ -185,13 +270,11 @@ export const acceptMergeRequest = async (gitlabApi: GitlabApi, mergeRequest: Mer
 			};
 		}
 
-		const approvals = await gitlabApi.getMergeRequestApprovals(mergeRequestInfo.project_id, mergeRequestInfo.iid);
-		if (approvals.approvals_left > 0) {
+		if (!containsAssignedUser(mergeRequestInfo, user)) {
 			return {
-				kind: AcceptMergeRequestResultKind.WaitingForApprovals,
+				kind: AcceptMergeRequestResultKind.ReassignedMergeRequest,
 				mergeRequestInfo,
 				user,
-				approvals,
 			};
 		}
 
@@ -224,6 +307,16 @@ export const acceptMergeRequest = async (gitlabApi: GitlabApi, mergeRequest: Mer
 			};
 		}
 
+		const approvals = await gitlabApi.getMergeRequestApprovals(mergeRequestInfo.project_id, mergeRequestInfo.iid);
+		if (approvals.approvals_left > 0) {
+			return {
+				kind: AcceptMergeRequestResultKind.WaitingForApprovals,
+				mergeRequestInfo,
+				user,
+				approvals,
+			};
+		}
+
 		if (mergeRequestInfo.diverged_commits_count > 0) {
 			if (numberOfRebasingRetries <= 0 && mergeRequestInfo.merge_error !== null) {
 				console.log(`[MR][${mergeRequestInfo.iid}] Merge error after rebase`);
@@ -354,57 +447,6 @@ export const acceptMergeRequest = async (gitlabApi: GitlabApi, mergeRequest: Mer
 			}
 		}
 
-		console.log(`[MR][${mergeRequestInfo.iid}] Calling merge request`);
-		const response = await gitlabApi.sendRawRequest(`/api/v4/projects/${mergeRequestInfo.project_id}/merge_requests/${mergeRequestInfo.iid}/merge`, RequestMethod.Put, {
-			should_remove_source_branch: options.removeBranchAfterMerge,
-			sha: mergeRequestInfo.diff_refs.head_sha,
-			squash: mergeRequestInfo.labels.includes(options.skipSquashingLabel) ? false : options.squashMergeRequest,
-			squash_commit_message: `${mergeRequestInfo.title} (!${mergeRequestInfo.iid})`,
-			merge_commit_message: `${mergeRequestInfo.title} (!${mergeRequestInfo.iid})`,
-		});
-
-		if (response.status === 405) {
-			console.log(`[MR][${mergeRequestInfo.iid}] 405 - cannot be merged`);
-			continue;
-		}
-
-		if (response.status === 406) {
-			console.log(`[MR][${mergeRequestInfo.iid}] 406 - already merged`);
-			continue;
-		}
-
-		if (response.status === 409) {
-			console.log(`[MR][${mergeRequestInfo.iid}] 409 - SHA does not match HEAD of source branch`);
-			continue;
-		}
-
-		if (response.status === 401) {
-			return {
-				kind: AcceptMergeRequestResultKind.Unauthorized,
-				mergeRequestInfo,
-				user,
-			};
-		}
-
-		if (response.status !== 200) {
-			throw new Error(`Unsupported response status ${response.status}`);
-		}
-
-		const data = await response.json();
-		if (typeof data !== 'object' && data.id === undefined) {
-			console.error('response', data);
-			throw new Error('Invalid response');
-		}
-
-		if (!containsLabel(mergeRequestInfo.labels, BotLabels.Accepting)) {
-			tasks.push(
-				gitlabApi.updateMergeRequest(mergeRequestInfo.project_id, mergeRequestInfo.iid, {
-					labels: [...filterBotLabels(mergeRequestInfo.labels), BotLabels.Accepting].join(','),
-				}),
-			);
-		}
-
-		console.log(`[MR][${mergeRequestInfo.iid}] Merge request is processing`);
 		await Promise.all(tasks);
 	}
 };

src/Queue.ts

@@ -1,6 +1,6 @@
 export enum JobPriority {
-	HI,
-	NORMAL,
+	HIGH = 'high',
+	NORMAL = 'normal',
 }
 
 interface Jobs {
@@ -11,34 +11,45 @@ export class Queue {
 
 	private promise?: Promise<void>;
 	private jobs: { [key in JobPriority]: Jobs } = {
-		[JobPriority.HI]: {},
+		[JobPriority.HIGH]: {},
 		[JobPriority.NORMAL]: {},
 	};
 
-	public hasJob(
+	public setJobPriority(
 		jobId: string,
 		jobPriority: JobPriority,
 	): boolean {
-		const currentJobPriority = this.getPriorityByJobId(jobId);
+		const currentJobPriority = this.findPriorityByJobId(jobId);
 		if (currentJobPriority === null) {
 			return false;
 		}
 
-		if (currentJobPriority === JobPriority.NORMAL && jobPriority === JobPriority.HI) {
-			console.log(`[job][${jobId}] Job has been moved to the prioritized queue`);
+		if (currentJobPriority === JobPriority.NORMAL && jobPriority === JobPriority.HIGH) {
 			this.jobs[jobPriority][jobId] = this.jobs[currentJobPriority][jobId];
 			delete this.jobs[currentJobPriority][jobId];
 		}
 
 		return true;
 	}
 
+	public findPriorityByJobId(jobId: string): JobPriority | null {
+		if (typeof this.jobs[JobPriority.HIGH][jobId] !== 'undefined') {
+			return JobPriority.HIGH;
+		}
+
+		if (typeof this.jobs[JobPriority.NORMAL][jobId] !== 'undefined') {
+			return JobPriority.NORMAL;
+		}
+
+		return null;
+	}
+
 	public runJob<T extends Promise<any>>(
 		jobId: string,
 		job: () => T,
 		jobPriority: JobPriority,
 	): T {
-		const currentJobPriority = this.getPriorityByJobId(jobId);
+		const currentJobPriority = this.findPriorityByJobId(jobId);
 		if (currentJobPriority !== null) {
 			throw new Error(`JobId ${jobId} is already in queue`);
 		}
@@ -51,7 +62,7 @@ export class Queue {
 					reject(e);
 				}
 
-				const runtimeJobPriority = this.getPriorityByJobId(jobId);
+				const runtimeJobPriority = this.findPriorityByJobId(jobId);
 				if (runtimeJobPriority === null) {
 					throw new Error(`JobId ${jobId} not found`);
 				}
@@ -65,8 +76,8 @@ export class Queue {
 		if (this.promise === undefined) {
 			this.promise = new Promise(async (resolve, reject) => {
 				while (true) {
-					let jobIds = Object.keys(this.jobs[JobPriority.HI]);
-					let priority = JobPriority.HI;
+					let jobIds = Object.keys(this.jobs[JobPriority.HIGH]);
+					let priority = JobPriority.HIGH;
 
 					if (jobIds.length === 0) {
 						jobIds = Object.keys(this.jobs[JobPriority.NORMAL]);
@@ -93,16 +104,4 @@ export class Queue {
 		return jobPromise as T;
 	}
 
-	private getPriorityByJobId(jobId: string): JobPriority | null {
-		if (typeof this.jobs[JobPriority.HI][jobId] !== 'undefined') {
-			return JobPriority.HI;
-		}
-
-		if (typeof this.jobs[JobPriority.NORMAL][jobId] !== 'undefined') {
-			return JobPriority.NORMAL;
-		}
-
-		return null;
-	}
-
 }