Improve typing and fix workflow actor ID checking

Signed-off-by: Kyle Harding <kyle@balena.io>
product-os · Dec 4, 2024 · 88a13fa · 88a13fa
1 parent 8ca916e
commit 88a13fa
Show file tree

Hide file tree

Showing 6 changed files with 86 additions and 68 deletions.
diff --git a/src/client.ts b/src/client.ts
@@ -1,9 +1,10 @@
-// export async function whoAmI(context: any): Promise<any> {
-// 	const query = `query { viewer { databaseId login } }`;
-// 	const { viewer } = await context.octokit.graphql(query);
-// 	console.log(`Authenticated as: ${viewer.login} (${viewer.databaseId})`);
-// 	return { login: viewer.login, id: viewer.databaseId };
-// }
+import type {
+	WorkflowRun,
+	PullRequestReview,
+	IssueComment,
+} from '@octokit/webhooks-types';
+import type { components } from '@octokit/openapi-types';
+type PendingDeployment = components['schemas']['pending-deployment'];
 
 // // https://octokit.github.io/rest.js/v21/#repos-get-collaborator-permission-level
 // // https://docs.github.com/en/rest/collaborators/collaborators#list-repository-collaborators
@@ -56,11 +57,12 @@
 // 	await context.octokit.reactions.createForPullRequestReviewComment(request);
 // }
 
-// https://docs.github.com/en/rest/deployments/deployments#list-deployments
+// // https://octokit.github.io/rest.js/v21/#repos-list-deployments
+// // https://docs.github.com/en/rest/deployments/deployments#list-deployments
 // export async function listDeployments(
 //   context: any,
 //   sha: string
-// ): Promise<any[]> {
+// ): Promise<Deployment[]> {
 //   const request = context.repo({
 //     sha,
 //   });
@@ -88,7 +90,7 @@
 export async function listPullRequestReviews(
 	context: any,
 	prNumber: number,
-): Promise<any[]> {
+): Promise<PullRequestReview[]> {
 	const request = context.repo({
 		pull_number: prNumber,
 	});
@@ -102,7 +104,7 @@ export async function listPullRequestReviews(
 export async function listWorkflowRuns(
 	context: any,
 	headSha: string,
-): Promise<any> {
+): Promise<WorkflowRun[]> {
 	// what is the status "requested" used for?
 	const request = context.repo({
 		status: 'waiting',
@@ -111,7 +113,6 @@ export async function listWorkflowRuns(
 	const {
 		data: { workflow_runs: runs },
 	} = await context.octokit.rest.actions.listWorkflowRunsForRepo(request);
-	// console.log(JSON.stringify(runs, null, 2))
 	return runs;
 }
 
@@ -120,7 +121,7 @@ export async function listWorkflowRuns(
 export async function listPendingDeployments(
 	context: any,
 	runId: number,
-): Promise<any[]> {
+): Promise<PendingDeployment[]> {
 	const request = context.repo({
 		run_id: runId,
 	});
@@ -130,6 +131,20 @@ export async function listPendingDeployments(
 	return deployments;
 }
 
+// // https://docs.github.com/en/rest/deployments/deployments#get-a-deployment
+// // https://octokit.github.io/rest.js/v21/#repos-get-deployment
+// export async function getDeployment(
+// 	context: any,
+// 	deploymentId: number,
+// ): Promise<Deployment> {
+// 	const request = context.repo({
+// 		deployment_id: deploymentId,
+// 	});
+// 	const { data: deployment } =
+// 		await context.octokit.rest.repos.getDeployment(request);
+// 	return deployment;
+// }
+
 // https://octokit.github.io/rest.js/v21/#actions-review-custom-gates-for-run
 // https://docs.github.com/en/rest/actions/workflow-runs#review-custom-deployment-protection-rules-for-a-workflow-run
 export async function reviewWorkflowRun(
@@ -138,25 +153,23 @@ export async function reviewWorkflowRun(
 	environment: string,
 	state: string,
 	comment: string,
-): Promise<any> {
+): Promise<void> {
 	const request = context.repo({
 		run_id: runId,
 		environment_name: environment,
 		state,
 		comment,
 	});
 
-	const { data: review } =
-		await context.octokit.rest.actions.reviewCustomGatesForRun(request);
-	return review;
+	await context.octokit.rest.actions.reviewCustomGatesForRun(request);
 }
 
 // https://octokit.github.io/rest.js/v18/#issues-list-comments
 // https://docs.github.com/en/rest/issues/comments#list-issue-comments
 export async function listIssueComments(
 	context: any,
 	issueNumber: number,
-): Promise<any[]> {
+): Promise<IssueComment[]> {
 	const request = context.repo({
 		issue_number: issueNumber,
 	});
@@ -171,7 +184,7 @@ export async function createIssueComment(
 	context: any,
 	issueNumber: number,
 	body: string,
-): Promise<any> {
+): Promise<IssueComment> {
 	const request = context.repo({
 		issue_number: issueNumber,
 		body,

diff --git a/src/handlers/deployment-protection-rule.ts b/src/handlers/deployment-protection-rule.ts
@@ -78,10 +78,10 @@ export async function handleDeploymentProtectionRule(
 
 			const deployReview = reviews.find(
 				(review) =>
-					review.state !== 'CHANGES_REQUESTED' &&
+					['approved', 'commented'].includes(review.state) &&
 					review.commit_id === deployment.sha &&
 					review.user.id !== deployment.creator.id &&
-					review.body.startsWith('/deploy'),
+					review.body?.startsWith('/deploy'),
 			);
 
 			if (deployReview) {
@@ -120,7 +120,7 @@ async function filterIssueComments(
 	return comments.filter(
 		(c) =>
 			c.body.startsWith(startsWith) &&
-			c.performed_via_github_app.id === appId &&
+			c.performed_via_github_app?.id === appId &&
 			c.created_at === c.updated_at,
 	);
 }
diff --git a/src/handlers/pull-request-review.ts b/src/handlers/pull-request-review.ts
@@ -1,5 +1,8 @@
 import type { Context } from 'probot';
-import type { PullRequestReviewSubmittedEvent } from '@octokit/webhooks-types';
+import type {
+	PullRequestReviewSubmittedEvent,
+	WorkflowRun,
+} from '@octokit/webhooks-types';
 import * as GitHubClient from '../client.js';
 
 export async function handlePullRequestReview(context: Context) {
@@ -32,35 +35,44 @@ export async function handlePullRequestReview(context: Context) {
 		return;
 	}
 
-	const runs = await GitHubClient.listWorkflowRuns(context, review.commit_id);
+	const workflowRuns = await GitHubClient.listWorkflowRuns(
+		context,
+		review.commit_id,
+	);
 
-	for (const run of runs) {
-		const deployments = await GitHubClient.listPendingDeployments(
-			context,
-			run.id,
-		);
+	await Promise.all(
+		workflowRuns
+			.filter((workflowRun) => workflowRun.actor.id !== review.user.id)
+			.map(async (workflowRun: WorkflowRun) => {
+				const pendingDeployments = await GitHubClient.listPendingDeployments(
+					context,
+					workflowRun.id,
+				);
 
-		if (deployments.length === 0) {
-			context.log.info(
-				'No pending deployments found for workflow run %s',
-				run.id,
-			);
-			continue;
-		}
+				if (pendingDeployments.length === 0) {
+					context.log.info(
+						'No pending deployments found for workflow run %s',
+						workflowRun.id,
+					);
+					return;
+				}
 
-		const environments = deployments
-			.filter((deployment) => deployment.current_user_can_approve)
-			.filter((deployment) => deployment.creator.id !== review.user.id)
-			.map((deployment) => deployment.environment.name);
+				const environmentNames = pendingDeployments
+					.filter((deployment) => deployment.current_user_can_approve)
+					.filter((deployment) => deployment.environment.name !== undefined)
+					.map((deployment) => deployment.environment.name!);
 
-		for (const environment of environments) {
-			await GitHubClient.reviewWorkflowRun(
-				context,
-				run.id,
-				environment,
-				'approved',
-				`Approved by ${review.user.login} via [review](${review.html_url})`,
-			);
-		}
-	}
+				await Promise.all(
+					environmentNames.map((environmentName) =>
+						GitHubClient.reviewWorkflowRun(
+							context,
+							workflowRun.id,
+							environmentName,
+							'approved',
+							`Approved by ${review.user.login} via [review](${review.html_url})`,
+						),
+					),
+				);
+			}),
+	);
 }
diff --git a/test/handlers/deployment-protection-rule.test.ts b/test/handlers/deployment-protection-rule.test.ts
@@ -176,7 +176,7 @@ describe('Deployment Protection Rule Handler', () => {
 		expect(mock.pendingMocks()).toStrictEqual([]);
 	});
 
-	test('approves deployment for APPROVED pull request review', async () => {
+	test('approves deployment for approved pull request review', async () => {
 		const mock = nock('https://api.github.com')
 			.post('/app/installations/12345678/access_tokens')
 			.reply(200, { token: 'test', permissions: { issues: 'write' } })
@@ -187,7 +187,7 @@ describe('Deployment Protection Rule Handler', () => {
 				{
 					commit_id: 'test-sha',
 					body: '/deploy please',
-					state: 'APPROVED',
+					state: 'approved',
 					user: { login: 'test-user', id: 789 },
 				},
 			])
@@ -206,7 +206,7 @@ describe('Deployment Protection Rule Handler', () => {
 		expect(mock.pendingMocks()).toStrictEqual([]);
 	});
 
-	test('approves deployment for COMMENTED pull request review', async () => {
+	test('approves deployment for commented pull request review', async () => {
 		const mock = nock('https://api.github.com')
 			.post('/app/installations/12345678/access_tokens')
 			.reply(200, { token: 'test', permissions: { issues: 'write' } })
@@ -268,7 +268,7 @@ describe('Deployment Protection Rule Handler', () => {
 		expect(mock.pendingMocks()).toStrictEqual([]);
 	});
 
-	test('creates instructional comment for CHANGES_REQUESTED pull request review', async () => {
+	test('creates instructional comment for changes_requested pull request review', async () => {
 		const mock = nock('https://api.github.com')
 			.post('/app/installations/12345678/access_tokens')
 			.reply(200, { token: 'test', permissions: { issues: 'write' } })
@@ -279,7 +279,7 @@ describe('Deployment Protection Rule Handler', () => {
 				{
 					commit_id: 'test-sha',
 					body: '/deploy please',
-					state: 'CHANGES_REQUESTED',
+					state: 'changes_requested',
 					user: { login: 'test-user', id: 789 },
 				},
 			])

diff --git a/test/handlers/pull-request-review.test.ts b/test/handlers/pull-request-review.test.ts
@@ -22,6 +22,7 @@ const testFixtures = {
 				login: 'test-user',
 				id: 789,
 			},
+			html_url: 'https://github.com/test-org/test-repo/pull/123/reviews/456',
 		},
 		installation: { id: 12345678 },
 		repository: {
@@ -60,12 +61,11 @@ describe('Pull Request Review Handler', () => {
 			.reply(200, { token: 'test', permissions: { issues: 'write' } })
 			.get('/repos/test-org/test-repo/actions/runs')
 			.query(true)
-			.reply(200, { workflow_runs: [{ id: 1234 }] })
+			.reply(200, { workflow_runs: [{ id: 1234, actor: { id: 123 } }] })
 			.get('/repos/test-org/test-repo/actions/runs/1234/pending_deployments')
 			.reply(200, [
 				{
 					environment: { name: 'test' },
-					creator: { id: 123 },
 					current_user_can_approve: true,
 				},
 			])
@@ -119,7 +119,7 @@ describe('Pull Request Review Handler', () => {
 		expect(nock.pendingMocks()).toStrictEqual([]);
 	});
 
-	test('ignores review by deployment creator', async () => {
+	test('ignores review by workflow run actor', async () => {
 		const payload = {
 			...testFixtures.pull_request_review,
 			review: {
@@ -136,15 +136,7 @@ describe('Pull Request Review Handler', () => {
 			.reply(200, { token: 'test', permissions: { issues: 'write' } })
 			.get('/repos/test-org/test-repo/actions/runs')
 			.query(true)
-			.reply(200, { workflow_runs: [{ id: 1234 }] })
-			.get('/repos/test-org/test-repo/actions/runs/1234/pending_deployments')
-			.reply(200, [
-				{
-					environment: { name: 'test' },
-					creator: { id: 123 },
-					current_user_can_approve: true,
-				},
-			]);
+			.reply(200, { workflow_runs: [{ id: 1234, actor: { id: 123 } }] });
 
 		await probot.receive({
 			name: 'pull_request_review',
@@ -176,7 +168,7 @@ describe('Pull Request Review Handler', () => {
 			.reply(200, { token: 'test', permissions: { issues: 'write' } })
 			.get('/repos/test-org/test-repo/actions/runs')
 			.query(true)
-			.reply(200, { workflow_runs: [{ id: 1234 }] })
+			.reply(200, { workflow_runs: [{ id: 1234, actor: { id: 123 } }] })
 			.get('/repos/test-org/test-repo/actions/runs/1234/pending_deployments')
 			.reply(200, []);
 
@@ -194,7 +186,7 @@ describe('Pull Request Review Handler', () => {
 			.reply(200, { token: 'test', permissions: { issues: 'write' } })
 			.get('/repos/test-org/test-repo/actions/runs')
 			.query(true)
-			.reply(200, { workflow_runs: [{ id: 1234 }] })
+			.reply(200, { workflow_runs: [{ id: 1234, actor: { id: 123 } }] })
 			.get('/repos/test-org/test-repo/actions/runs/1234/pending_deployments')
 			.reply(200, [
 				{ environment: { name: 'test' }, current_user_can_approve: false },

diff --git a/tsconfig.build.json b/tsconfig.build.json
@@ -8,6 +8,7 @@
 		"strict": true,
 		"strictPropertyInitialization": false,
 		"strictFunctionTypes": false,
+		"strictNullChecks": true,
 		"useUnknownInCatchVariables": false,
 		"noImplicitThis": false,
 		"noUnusedParameters": true,