From ba8db09ccf18c566e1864d4924c34a37a71410ce Mon Sep 17 00:00:00 2001 From: Alessandro Dalfovo Date: Fri, 5 Aug 2022 11:00:37 +0200 Subject: [PATCH 1/9] Introduce reviewers Signed-off-by: Alessandro Dalfovo --- .../builtin/publish/githubPullRequest.ts | 51 +++++++++++++++++-- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index f8f9b22d61..cfaee62804 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -33,14 +33,14 @@ export type Encoding = 'utf-8' | 'base64'; class GithubResponseError extends CustomErrorBase {} /** @public */ -export interface OctokitWithPullRequestPluginClient { +export type OctokitWithPullRequestPluginClient = Octokit & { createPullRequest(options: createPullRequest.Options): Promise<{ data: { html_url: string; number: number; }; } | null>; -} +}; /** * The options passed to the client factory function. @@ -117,6 +117,8 @@ export const createPublishGithubPullRequestAction = ({ targetPath?: string; sourcePath?: string; token?: string; + reviewers?: string[]; + teamReviewers?: string[]; }>({ id: 'publish:github:pull-request', schema: { @@ -165,6 +167,22 @@ export const createPublishGithubPullRequestAction = ({ type: 'string', description: 'The token to use for authorization to GitHub', }, + reviewers: { + title: 'Pull Request Reviewers', + type: 'array', + items: { + type: 'string', + }, + description: 'Pull Request Reviewers', + }, + teamReviewers: { + title: 'Pull Request Team Reviewers', + type: 'array', + items: { + type: 'string', + }, + description: 'Pull Request Team Reviewers', + }, }, }, output: { @@ -194,6 +212,8 @@ export const createPublishGithubPullRequestAction = ({ targetPath, sourcePath, token: providedToken, + reviewers, + teamReviewers, } = ctx.input; const { owner, repo, host } = parseRepoUrl(repoUrl, integrations); @@ -259,8 +279,33 @@ export const createPublishGithubPullRequestAction = ({ throw new GithubResponseError('null response from Github'); } + const pullRequestNumber = response.data.number; + if (reviewers !== null || teamReviewers !== null) { + try { + const result = await client.rest.pulls.requestReviewers({ + owner, + repo, + pull_number: pullRequestNumber, + reviewers: reviewers, + team_reviewers: teamReviewers, + }); + const addedUsers = result.data.requested_reviewers ?? []; + const addedTeams = result.data.requested_teams ?? []; + ctx.logger.info( + `Added users [${addedUsers.join( + ',', + )}] and teams [${addedTeams.join(',')}] as reviewers`, + ); + } catch (e) { + ctx.logger.error( + `Failure when adding reviewers to Pull request ${pullRequestNumber}`, + e, + ); + } + } + ctx.output('remoteUrl', response.data.html_url); - ctx.output('pullRequestNumber', response.data.number); + ctx.output('pullRequestNumber', pullRequestNumber); } catch (e) { throw new GithubResponseError('Pull request creation failed', e); } From 4a7315c5f4382a60a9724a9cb7e222dc5ce1399f Mon Sep 17 00:00:00 2001 From: Francesco Saltori Date: Fri, 5 Aug 2022 12:28:21 +0200 Subject: [PATCH 2/9] Add tests Signed-off-by: Francesco Saltori --- .../builtin/publish/githubPullRequest.test.ts | 95 +++++++++++++++++++ .../builtin/publish/githubPullRequest.ts | 2 +- 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts index d030430924..bf857bc34f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts @@ -62,6 +62,11 @@ describe('createPublishGithubPullRequestAction', () => { }, }; }), + rest: { + pulls: { + requestReviewers: jest.fn(async (_: any) => ({ data: {} })) + } + } }; clientFactory = jest.fn(async () => fakeClient); const githubCredentialsProvider: GithubCredentialsProvider = { @@ -273,6 +278,96 @@ describe('createPublishGithubPullRequestAction', () => { }); }); + describe('with reviewers and teamReviewers', () => { + let input: GithubPullRequestActionInput; + let ctx: ActionContext; + + beforeEach(() => { + input = { + repoUrl: 'github.com?owner=myorg&repo=myrepo', + title: 'Create my new app', + branchName: 'new-app', + description: 'This PR is really good', + reviewers: ['foobar'], + teamReviewers: ['team-foo'] + }; + + ctx = { + createTemporaryDirectory: jest.fn(), + output: jest.fn(), + logger: getRootLogger(), + logStream: new Writable(), + input, + workspacePath, + }; + }); + + it('creates a pull request and requests a review from the given reviewers', async () => { + await instance.handler(ctx); + + expect(fakeClient.createPullRequest).toBeCalled(); + expect(fakeClient.rest.pulls.requestReviewers).toBeCalledWith({ + owner: 'myorg', + repo: 'myrepo', + pull_number: 123, + reviewers: ['foobar'], + team_reviewers: ['team-foo'], + }) + }); + + it('creates outputs for the pull request url and number even if requesting reviewers fails', async () => { + fakeClient.rest.pulls.requestReviewers.mockImplementation(() => { throw new Error('a random error') }) + + await instance.handler(ctx); + + expect(ctx.output).toHaveBeenCalledWith( + 'remoteUrl', + 'https://github.com/myorg/myrepo/pull/123', + ); + expect(ctx.output).toHaveBeenCalledWith('pullRequestNumber', 123); + }); + + afterEach(() => { + mockFs.restore(); + jest.resetAllMocks(); + }); + }) + + describe('with no reviewers and teamReviewers', () => { + let input: GithubPullRequestActionInput; + let ctx: ActionContext; + + beforeEach(() => { + input = { + repoUrl: 'github.com?owner=myorg&repo=myrepo', + title: 'Create my new app', + branchName: 'new-app', + description: 'This PR is really good', + }; + + ctx = { + createTemporaryDirectory: jest.fn(), + output: jest.fn(), + logger: getRootLogger(), + logStream: new Writable(), + input, + workspacePath, + }; + }); + + it('does not call the API endpoint for requesting reviewers', async () => { + await instance.handler(ctx); + + expect(fakeClient.createPullRequest).toBeCalled(); + expect(fakeClient.rest.pulls.requestReviewers).not.toBeCalled(); + }); + + afterEach(() => { + mockFs.restore(); + jest.resetAllMocks(); + }); + }) + describe('with executable file mode 755', () => { let input: GithubPullRequestActionInput; let ctx: ActionContext; diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index cfaee62804..67379e8d70 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -280,7 +280,7 @@ export const createPublishGithubPullRequestAction = ({ } const pullRequestNumber = response.data.number; - if (reviewers !== null || teamReviewers !== null) { + if (reviewers || teamReviewers) { try { const result = await client.rest.pulls.requestReviewers({ owner, From 1cb5e2cbfe137acc71b4055d3de61a3164c71ac2 Mon Sep 17 00:00:00 2001 From: Francesco Saltori Date: Fri, 5 Aug 2022 12:56:35 +0200 Subject: [PATCH 3/9] Refactor tests a bit and fix typings Signed-off-by: Francesco Saltori --- .../builtin/publish/githubPullRequest.test.ts | 73 +++++++------------ 1 file changed, 26 insertions(+), 47 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts index bf857bc34f..dd493495d8 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts @@ -26,7 +26,6 @@ import { resolve as resolvePath } from 'path'; import { Writable } from 'stream'; import { ActionContext, TemplateAction } from '../../types'; import { - CreateGithubPullRequestClientFactoryInput, createPublishGithubPullRequestAction, OctokitWithPullRequestPluginClient, } from './githubPullRequest'; @@ -42,11 +41,12 @@ type GithubPullRequestActionInput = ReturnType< describe('createPublishGithubPullRequestAction', () => { let instance: TemplateAction; - let fakeClient: OctokitWithPullRequestPluginClient; - - let clientFactory: ( - input: CreateGithubPullRequestClientFactoryInput, - ) => Promise; + let fakeClient: { + createPullRequest: jest.Mock; + rest: { + pulls: { requestReviewers: jest.Mock }; + }; + }; beforeEach(() => { const integrations = ScmIntegrations.fromConfig(new ConfigReader({})); @@ -64,11 +64,13 @@ describe('createPublishGithubPullRequestAction', () => { }), rest: { pulls: { - requestReviewers: jest.fn(async (_: any) => ({ data: {} })) - } - } + requestReviewers: jest.fn(async (_: any) => ({ data: {} })), + }, + }, }; - clientFactory = jest.fn(async () => fakeClient); + const clientFactory = jest.fn( + async () => fakeClient as unknown as OctokitWithPullRequestPluginClient, + ); const githubCredentialsProvider: GithubCredentialsProvider = { getCredentials: jest.fn(), }; @@ -80,6 +82,11 @@ describe('createPublishGithubPullRequestAction', () => { }); }); + afterEach(() => { + mockFs.restore(); + jest.resetAllMocks(); + }); + describe('with no sourcePath', () => { let input: GithubPullRequestActionInput; let ctx: ActionContext; @@ -106,6 +113,7 @@ describe('createPublishGithubPullRequestAction', () => { workspacePath, }; }); + it('creates a pull request', async () => { await instance.handler(ctx); @@ -140,10 +148,6 @@ describe('createPublishGithubPullRequestAction', () => { ); expect(ctx.output).toHaveBeenCalledWith('pullRequestNumber', 123); }); - afterEach(() => { - mockFs.restore(); - jest.resetAllMocks(); - }); }); describe('with sourcePath', () => { @@ -176,11 +180,6 @@ describe('createPublishGithubPullRequestAction', () => { }; }); - afterEach(() => { - mockFs.restore(); - jest.resetAllMocks(); - }); - it('creates a pull request with only relevant files', async () => { await instance.handler(ctx); @@ -272,10 +271,6 @@ describe('createPublishGithubPullRequestAction', () => { ); expect(ctx.output).toHaveBeenCalledWith('pullRequestNumber', 123); }); - afterEach(() => { - mockFs.restore(); - jest.resetAllMocks(); - }); }); describe('with reviewers and teamReviewers', () => { @@ -289,7 +284,7 @@ describe('createPublishGithubPullRequestAction', () => { branchName: 'new-app', description: 'This PR is really good', reviewers: ['foobar'], - teamReviewers: ['team-foo'] + teamReviewers: ['team-foo'], }; ctx = { @@ -312,12 +307,14 @@ describe('createPublishGithubPullRequestAction', () => { pull_number: 123, reviewers: ['foobar'], team_reviewers: ['team-foo'], - }) + }); }); it('creates outputs for the pull request url and number even if requesting reviewers fails', async () => { - fakeClient.rest.pulls.requestReviewers.mockImplementation(() => { throw new Error('a random error') }) - + fakeClient.rest.pulls.requestReviewers.mockImplementation(() => { + throw new Error('a random error'); + }); + await instance.handler(ctx); expect(ctx.output).toHaveBeenCalledWith( @@ -326,12 +323,7 @@ describe('createPublishGithubPullRequestAction', () => { ); expect(ctx.output).toHaveBeenCalledWith('pullRequestNumber', 123); }); - - afterEach(() => { - mockFs.restore(); - jest.resetAllMocks(); - }); - }) + }); describe('with no reviewers and teamReviewers', () => { let input: GithubPullRequestActionInput; @@ -361,12 +353,7 @@ describe('createPublishGithubPullRequestAction', () => { expect(fakeClient.createPullRequest).toBeCalled(); expect(fakeClient.rest.pulls.requestReviewers).not.toBeCalled(); }); - - afterEach(() => { - mockFs.restore(); - jest.resetAllMocks(); - }); - }) + }); describe('with executable file mode 755', () => { let input: GithubPullRequestActionInput; @@ -431,10 +418,6 @@ describe('createPublishGithubPullRequestAction', () => { ); expect(ctx.output).toHaveBeenCalledWith('pullRequestNumber', 123); }); - afterEach(() => { - mockFs.restore(); - jest.resetAllMocks(); - }); }); describe('with executable file mode 775', () => { @@ -500,9 +483,5 @@ describe('createPublishGithubPullRequestAction', () => { ); expect(ctx.output).toHaveBeenCalledWith('pullRequestNumber', 123); }); - afterEach(() => { - mockFs.restore(); - jest.resetAllMocks(); - }); }); }); From 5b9ab1542f6ae08638337ad514ab5b686ec6df6f Mon Sep 17 00:00:00 2001 From: Francesco Saltori Date: Mon, 8 Aug 2022 11:27:51 +0200 Subject: [PATCH 4/9] Refactor review request logic Signed-off-by: Francesco Saltori --- .../builtin/publish/githubPullRequest.ts | 64 +++++++++++++------ 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index 67379e8d70..6a37a89f33 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -27,6 +27,7 @@ import { createPullRequest } from 'octokit-plugin-create-pull-request'; import { resolveSafeChildPath } from '@backstage/backend-common'; import { getOctokitOptions } from '../github/helpers'; import { serializeDirectoryContents } from '../../../../lib/files'; +import { Logger } from 'winston'; export type Encoding = 'utf-8' | 'base64'; @@ -99,6 +100,12 @@ export interface CreateGithubPullRequestActionOptions { ) => Promise; } +type GithubPullRequest = { + owner: string; + repo: string; + number: number; +}; + /** * Creates a Github Pull Request action. * @public @@ -281,27 +288,14 @@ export const createPublishGithubPullRequestAction = ({ const pullRequestNumber = response.data.number; if (reviewers || teamReviewers) { - try { - const result = await client.rest.pulls.requestReviewers({ - owner, - repo, - pull_number: pullRequestNumber, - reviewers: reviewers, - team_reviewers: teamReviewers, - }); - const addedUsers = result.data.requested_reviewers ?? []; - const addedTeams = result.data.requested_teams ?? []; - ctx.logger.info( - `Added users [${addedUsers.join( - ',', - )}] and teams [${addedTeams.join(',')}] as reviewers`, - ); - } catch (e) { - ctx.logger.error( - `Failure when adding reviewers to Pull request ${pullRequestNumber}`, - e, - ); - } + const pullRequest = { owner, repo, number: pullRequestNumber }; + await requestReviewersOnPullRequest( + pullRequest, + reviewers, + teamReviewers, + client, + ctx.logger, + ); } ctx.output('remoteUrl', response.data.html_url); @@ -311,4 +305,32 @@ export const createPublishGithubPullRequestAction = ({ } }, }); + + async function requestReviewersOnPullRequest( + pr: GithubPullRequest, + reviewers: string[] | undefined, + teamReviewers: string[] | undefined, + client: Octokit, + logger: Logger, + ) { + try { + const result = await client.rest.pulls.requestReviewers({ + owner: pr.owner, + repo: pr.repo, + pull_number: pr.number, + reviewers, + team_reviewers: teamReviewers, + }); + const addedUsers = result.data.requested_reviewers?.join(', ') ?? ''; + const addedTeams = result.data.requested_teams?.join(', ') ?? ''; + logger.info( + `Added users [${addedUsers}] and teams [${addedTeams}] as reviewers to Pull request ${pr.number}`, + ); + } catch (e) { + logger.error( + `Failure when adding reviewers to Pull request ${pr.number}`, + e, + ); + } + } }; From 0bcb545875ba5a1ec272e1edf749b60aa204aa2c Mon Sep 17 00:00:00 2001 From: Francesco Saltori Date: Mon, 8 Aug 2022 11:32:51 +0200 Subject: [PATCH 5/9] Regenerate API report Signed-off-by: Francesco Saltori --- plugins/scaffolder-backend/api-report.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/plugins/scaffolder-backend/api-report.md b/plugins/scaffolder-backend/api-report.md index 615d6aa8fb..96a4d1e461 100644 --- a/plugins/scaffolder-backend/api-report.md +++ b/plugins/scaffolder-backend/api-report.md @@ -20,6 +20,7 @@ import { Knex } from 'knex'; import { LocationSpec } from '@backstage/plugin-catalog-backend'; import { Logger } from 'winston'; import { Observable } from '@backstage/types'; +import { Octokit } from 'octokit'; import { PluginDatabaseManager } from '@backstage/backend-common'; import { Schema } from 'jsonschema'; import { ScmIntegrationRegistry } from '@backstage/integration'; @@ -391,6 +392,8 @@ export const createPublishGithubPullRequestAction: ({ targetPath?: string | undefined; sourcePath?: string | undefined; token?: string | undefined; + reviewers?: string[] | undefined; + teamReviewers?: string[] | undefined; }>; // @public @@ -419,7 +422,7 @@ export const createPublishGitlabMergeRequestAction: (options: { branchName: string; targetPath: string; token?: string | undefined; - commitAction?: 'update' | 'create' | 'delete' | undefined; + commitAction?: 'update' | 'delete' | 'create' | undefined; projectid?: string | undefined; removeSourceBranch?: boolean | undefined; assignee?: string | undefined; @@ -523,15 +526,14 @@ export function fetchContents({ }): Promise; // @public (undocumented) -export interface OctokitWithPullRequestPluginClient { - // (undocumented) +export type OctokitWithPullRequestPluginClient = Octokit & { createPullRequest(options: createPullRequest.Options): Promise<{ data: { html_url: string; number: number; }; } | null>; -} +}; // @public export interface RouterOptions { From 692d5d3405470b5ef7777889b5db5e0b97b6c67e Mon Sep 17 00:00:00 2001 From: Francesco Saltori Date: Mon, 8 Aug 2022 11:42:03 +0200 Subject: [PATCH 6/9] Add changeset Signed-off-by: Francesco Saltori --- .changeset/tricky-ligers-move.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tricky-ligers-move.md diff --git a/.changeset/tricky-ligers-move.md b/.changeset/tricky-ligers-move.md new file mode 100644 index 0000000000..21aa7757ba --- /dev/null +++ b/.changeset/tricky-ligers-move.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': minor +--- + +Added `reviewers` and `teamReviewers` parameters to `publish:github:pull-request` action to add reviewers on the pull request created by the action From 3a0bdc3baa654d8ac0a228871b21ebad48c59979 Mon Sep 17 00:00:00 2001 From: Francesco Saltori Date: Mon, 8 Aug 2022 11:46:33 +0200 Subject: [PATCH 7/9] Improve params descriptions Signed-off-by: Francesco Saltori --- .../scaffolder/actions/builtin/publish/githubPullRequest.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index 6a37a89f33..51049e4357 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -180,7 +180,8 @@ export const createPublishGithubPullRequestAction = ({ items: { type: 'string', }, - description: 'Pull Request Reviewers', + description: + 'The users that will be added as reviewers to the pull request', }, teamReviewers: { title: 'Pull Request Team Reviewers', @@ -188,7 +189,8 @@ export const createPublishGithubPullRequestAction = ({ items: { type: 'string', }, - description: 'Pull Request Team Reviewers', + description: + 'The teams that will be added as reviewers to the pull request', }, }, }, From a9c1c1580ea834789b732d9bf508458b54e030ce Mon Sep 17 00:00:00 2001 From: Francesco Saltori Date: Mon, 8 Aug 2022 12:13:38 +0200 Subject: [PATCH 8/9] Fix API report Signed-off-by: Francesco Saltori --- plugins/scaffolder-backend/api-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/api-report.md b/plugins/scaffolder-backend/api-report.md index 96a4d1e461..de9a9e677f 100644 --- a/plugins/scaffolder-backend/api-report.md +++ b/plugins/scaffolder-backend/api-report.md @@ -422,7 +422,7 @@ export const createPublishGitlabMergeRequestAction: (options: { branchName: string; targetPath: string; token?: string | undefined; - commitAction?: 'update' | 'delete' | 'create' | undefined; + commitAction?: 'update' | 'create' | 'delete' | undefined; projectid?: string | undefined; removeSourceBranch?: boolean | undefined; assignee?: string | undefined; From 5ffaf1cc17b2edf5501faeff43ec1d00be23bf4c Mon Sep 17 00:00:00 2001 From: Francesco Saltori Date: Mon, 8 Aug 2022 15:50:29 +0200 Subject: [PATCH 9/9] Fix tests in CI Signed-off-by: Francesco Saltori --- .../actions/builtin/publish/githubPullRequest.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts index dd493495d8..1bc768e4bd 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts @@ -287,6 +287,8 @@ describe('createPublishGithubPullRequestAction', () => { teamReviewers: ['team-foo'], }; + mockFs({ [workspacePath]: {} }); + ctx = { createTemporaryDirectory: jest.fn(), output: jest.fn(), @@ -337,6 +339,8 @@ describe('createPublishGithubPullRequestAction', () => { description: 'This PR is really good', }; + mockFs({ [workspacePath]: {} }); + ctx = { createTemporaryDirectory: jest.fn(), output: jest.fn(),