From e784ba9de0afe79530e5db5677dd827c39692c30 Mon Sep 17 00:00:00 2001 From: Kevin Snyder Date: Fri, 4 Apr 2025 11:49:22 -0700 Subject: [PATCH 1/4] Use action context logger in Octokit client This will surface Octokit logs in the `OngoingTask` log stream. For example, Octokit will now surface logs when GitHub API rate limits are hit. Previously these were only visible in the backend logs and did not use the structured Backstage logger. Signed-off-by: Kevin Snyder --- .../src/actions/github.ts | 5 ++++- .../src/actions/githubActionsDispatch.ts | 22 ++++++++++--------- .../src/actions/githubAutolinks.ts | 22 ++++++++++--------- .../src/actions/githubBranchProtection.ts | 5 ++++- .../src/actions/githubDeployKey.ts | 5 ++++- .../src/actions/githubEnvironment.ts | 6 +++-- .../src/actions/githubIssuesLabel.ts | 22 ++++++++++--------- .../src/actions/githubPagesEnable.ts | 5 ++++- .../src/actions/githubRepoCreate.ts | 5 ++++- .../src/actions/githubRepoPush.ts | 5 ++++- .../src/actions/githubWebhook.ts | 22 ++++++++++--------- 11 files changed, 76 insertions(+), 48 deletions(-) diff --git a/plugins/scaffolder-backend-module-github/src/actions/github.ts b/plugins/scaffolder-backend-module-github/src/actions/github.ts index fd858f0830..62e0497dae 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/github.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/github.ts @@ -242,7 +242,10 @@ export function createPublishGithubAction(options: { owner, repo, }); - const client = new Octokit(octokitOptions); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); const { remoteUrl, repoContentsUrl } = await ctx.checkpoint({ key: `create.github.repo.${owner}.${repo}`, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.ts b/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.ts index 4faa53e6b8..40e48bb902 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.ts @@ -104,16 +104,18 @@ export function createGithubActionsDispatchAction(options: { throw new InputError('Invalid repository owner provided in repoUrl'); } - const client = new Octokit( - await getOctokitOptions({ - integrations, - host, - owner, - repo, - credentialsProvider: githubCredentialsProvider, - token: providedToken, - }), - ); + const octokitOptions = await getOctokitOptions({ + integrations, + host, + owner, + repo, + credentialsProvider: githubCredentialsProvider, + token: providedToken, + }); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); await ctx.checkpoint({ key: `create.workflow.dispatch.${owner}.${repo}.${workflowId}`, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.ts b/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.ts index 396d85708f..50ed052458 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.ts @@ -97,16 +97,18 @@ export function createGithubAutolinksAction(options: { throw new InputError('Invalid repository owner provided in repoUrl'); } - const client = new Octokit( - await getOctokitOptions({ - integrations, - host, - owner, - repo, - credentialsProvider: githubCredentialsProvider, - token, - }), - ); + const octokitOptions = await getOctokitOptions({ + integrations, + host, + owner, + repo, + credentialsProvider: githubCredentialsProvider, + token, + }); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); await ctx.checkpoint({ key: `create.auto.link.${owner}.${repo}`, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.ts b/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.ts index ae8965ef9a..79eebb4096 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.ts @@ -128,7 +128,10 @@ export function createGithubBranchProtectionAction(options: { owner, repo, }); - const client = new Octokit(octokitOptions); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); const defaultBranch = await ctx.checkpoint({ key: `read.default.branch.${owner}.${repo}`, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.ts b/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.ts index 7e674a3e52..a820050a87 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.ts @@ -124,7 +124,10 @@ export function createGithubDeployKeyAction(options: { repo, }); - const client = new Octokit(octokitOptions); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); await client.rest.repos.createDeployKey({ owner: owner, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.ts b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.ts index f93cb49946..522b6ed8b5 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.ts @@ -192,8 +192,10 @@ Wildcard characters will not match \`/\`. For example, to match tags that begin owner, repo, }); - - const client = new Octokit(octokitOptions); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); const repositoryId = await ctx.checkpoint({ key: `get.repo.${owner}.${repo}`, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.ts b/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.ts index 7dd116be79..f91bd01a73 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.ts @@ -89,16 +89,18 @@ export function createGithubIssuesLabelAction(options: { throw new InputError('Invalid repository owner provided in repoUrl'); } - const client = new Octokit( - await getOctokitOptions({ - integrations, - credentialsProvider: githubCredentialsProvider, - host, - owner, - repo, - token: providedToken, - }), - ); + const octokitOptions = await getOctokitOptions({ + integrations, + credentialsProvider: githubCredentialsProvider, + host, + owner, + repo, + token: providedToken, + }); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); try { await ctx.checkpoint({ diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.ts index 7efee89a5d..01830a252d 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.ts @@ -112,7 +112,10 @@ export function createGithubPagesEnableAction(options: { owner, repo, }); - const client = new Octokit(octokitOptions); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); ctx.logger.info( `Attempting to enable GitHub Pages for ${owner}/${repo} with "${buildType}" build type, on source branch "${sourceBranch}" and source path "${sourcePath}"`, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.ts b/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.ts index 6899b3dff8..442af4057c 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.ts @@ -197,7 +197,10 @@ export function createGithubRepoCreateAction(options: { owner, repo, }); - const client = new Octokit(octokitOptions); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); const remoteUrl = await ctx.checkpoint({ key: `create.repo.and.topics.${owner}.${repo}`, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.ts b/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.ts index 1923a0bd35..14c12b84f9 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.ts @@ -158,7 +158,10 @@ export function createGithubRepoPushAction(options: { repo, }); - const client = new Octokit(octokitOptions); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); const targetRepo = await client.rest.repos.get({ owner, repo }); diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.ts b/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.ts index 39a659cd0a..7345f7dd9f 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.ts @@ -149,16 +149,18 @@ export function createGithubWebhookAction(options: { throw new InputError('Invalid repository owner provided in repoUrl'); } - const client = new Octokit( - await getOctokitOptions({ - integrations, - credentialsProvider: githubCredentialsProvider, - host, - owner, - repo, - token: providedToken, - }), - ); + const octokitOptions = await getOctokitOptions({ + integrations, + credentialsProvider: githubCredentialsProvider, + host, + owner, + repo, + token: providedToken, + }); + const client = new Octokit({ + ...octokitOptions, + log: ctx.logger, + }); // If this is a dry run, log and return if (ctx.isDryRun) { From 995d0249e9e202bce35a05c4c2218fab2933640f Mon Sep 17 00:00:00 2001 From: Kevin Snyder Date: Mon, 7 Apr 2025 11:56:16 -0700 Subject: [PATCH 2/4] Add tests Signed-off-by: Kevin Snyder --- .../src/actions/github.test.ts | 22 ++++++++--- .../src/actions/githubActionsDispatch.test.ts | 22 ++++++++--- .../src/actions/githubAutolinks.test.ts | 37 ++++++++++++++++--- .../actions/githubBranchProtection.test.ts | 24 +++++++++--- .../src/actions/githubDeployKey.test.ts | 23 +++++++++--- .../src/actions/githubEnvironment.test.ts | 24 +++++++++--- .../src/actions/githubIssuesLabel.test.ts | 22 ++++++++--- .../src/actions/githubPagesEnable.test.ts | 23 +++++++++--- .../src/actions/githubRepoCreate.test.ts | 22 ++++++++--- .../src/actions/githubRepoPush.test.ts | 23 +++++++++--- .../src/actions/githubWebhook.test.ts | 22 ++++++++--- 11 files changed, 208 insertions(+), 56 deletions(-) diff --git a/plugins/scaffolder-backend-module-github/src/actions/github.test.ts b/plugins/scaffolder-backend-module-github/src/actions/github.test.ts index 30945d92f2..97867226c6 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/github.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/github.test.ts @@ -54,6 +54,9 @@ const initRepoAndPushMocked = initRepoAndPush as jest.Mock< Promise<{ commitHash: string }> >; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { users: { @@ -81,11 +84,7 @@ const mockOctokit = { request: jest.fn(), }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('publish:github', () => { @@ -114,6 +113,7 @@ describe('publish:github', () => { }); beforeEach(() => { + octokitMock.mockImplementation(() => mockOctokit); initRepoAndPushMocked.mockResolvedValue({ commitHash: '220f19cc36b551763d157f1b5e4a4b446165dbd6', }); @@ -139,6 +139,18 @@ describe('publish:github', () => { afterEach(jest.resetAllMocks); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should fail to create if the team is not found in the org', async () => { mockOctokit.rest.users.getByUsername.mockResolvedValue({ data: { type: 'Organization' }, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.test.ts index 7f0d9b0490..6924d909d9 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.test.ts @@ -24,6 +24,9 @@ import { TemplateAction } from '@backstage/plugin-scaffolder-node'; import { createMockActionContext } from '@backstage/plugin-scaffolder-node-test-utils'; import { createGithubActionsDispatchAction } from './githubActionsDispatch'; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { actions: { @@ -32,11 +35,7 @@ const mockOctokit = { }, }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('github:actions:dispatch', () => { @@ -63,6 +62,7 @@ describe('github:actions:dispatch', () => { beforeEach(() => { jest.resetAllMocks(); + octokitMock.mockImplementation(() => mockOctokit); githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); action = createGithubActionsDispatchAction({ @@ -71,6 +71,18 @@ describe('github:actions:dispatch', () => { }); }); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should call the githubApis for creating WorkflowDispatch without an input object', async () => { mockOctokit.rest.actions.createWorkflowDispatch.mockResolvedValue({ data: { diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.test.ts index 56115c156c..0c63376a15 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.test.ts @@ -25,6 +25,9 @@ import { createMockActionContext } from '@backstage/plugin-scaffolder-node-test- import { TemplateAction } from '@backstage/plugin-scaffolder-node'; import { createGithubAutolinksAction } from './githubAutolinks'; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { repos: { @@ -33,11 +36,7 @@ const mockOctokit = { }, }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('github:autolinks:create', () => { @@ -55,6 +54,34 @@ describe('github:autolinks:create', () => { let action: TemplateAction; const workspacePath = createMockDirectory().resolve('workspace'); + it('should pass context logger to Octokit client', async () => { + githubCredentialsProvider = + DefaultGithubCredentialsProvider.fromIntegrations(integrations); + action = createGithubAutolinksAction({ + integrations, + githubCredentialsProvider, + }); + + octokitMock.mockImplementation(() => mockOctokit); + + const mockContext = createMockActionContext({ + input: { + repoUrl: 'github.com?repo=repo&owner=owner', + }, + workspacePath, + }); + + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should call the githubApis for creating alphanumeric autolink reference', async () => { githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.test.ts index 8e672af725..5f26b9d298 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.test.ts @@ -20,6 +20,9 @@ import { TemplateAction } from '@backstage/plugin-scaffolder-node'; import { ConfigReader } from '@backstage/config'; import { ScmIntegrations } from '@backstage/integration'; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { repos: { @@ -29,13 +32,8 @@ const mockOctokit = { }, }, }; - jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('github:branch-protection:create', () => { @@ -59,6 +57,8 @@ describe('github:branch-protection:create', () => { }); beforeEach(() => { + octokitMock.mockImplementation(() => mockOctokit); + mockOctokit.rest.repos.get.mockResolvedValue({ data: { default_branch: 'master', @@ -72,6 +72,18 @@ describe('github:branch-protection:create', () => { afterEach(jest.resetAllMocks); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should work with default params', async () => { await action.handler(mockContext); diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.test.ts index 798aa7705e..e1e7885a15 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.test.ts @@ -20,6 +20,9 @@ import { createMockActionContext } from '@backstage/plugin-scaffolder-node-test- import { ConfigReader } from '@backstage/config'; import { ScmIntegrations } from '@backstage/integration'; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { actions: { @@ -32,11 +35,7 @@ const mockOctokit = { }, }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); const publicKey = '2Sg8iYjAxxmI2LvUXpJjkYrMxURPc8r+dB7TJyvvcCU='; @@ -66,11 +65,25 @@ describe('github:deployKey:create', () => { beforeEach(() => { jest.resetAllMocks(); + octokitMock.mockImplementation(() => mockOctokit); + action = createGithubDeployKeyAction({ integrations, }); }); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should work happy path', async () => { mockOctokit.rest.actions.getRepoPublicKey.mockResolvedValue({ data: { diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts index 1a4ffa5ea4..ebef47a06b 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts @@ -22,6 +22,10 @@ import { ScmIntegrations } from '@backstage/integration'; import { CatalogApi } from '@backstage/catalog-client'; import { mockCredentials, mockServices } from '@backstage/backend-test-utils'; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; + const mockOctokit = { rest: { actions: { @@ -48,11 +52,7 @@ const mockCatalogClient: Partial = { }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); jest.mock('@backstage/catalog-client', () => ({ @@ -91,6 +91,8 @@ describe('github:environment:create', () => { }); beforeEach(() => { + octokitMock.mockImplementation(() => mockOctokit); + mockOctokit.rest.actions.getEnvironmentPublicKey.mockResolvedValue({ data: { key: publicKey, @@ -138,6 +140,18 @@ describe('github:environment:create', () => { afterEach(jest.resetAllMocks); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should work happy path', async () => { await action.handler(mockContext); diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.test.ts index af1d7d552c..e792c7d91f 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.test.ts @@ -31,6 +31,9 @@ jest.mock('../util', () => { }; }); +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { issues: { @@ -39,11 +42,7 @@ const mockOctokit = { }, }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('github:issues:label', () => { @@ -71,6 +70,7 @@ describe('github:issues:label', () => { beforeEach(() => { jest.resetAllMocks(); + octokitMock.mockImplementation(() => mockOctokit); githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); action = createGithubIssuesLabelAction({ @@ -79,6 +79,18 @@ describe('github:issues:label', () => { }); }); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should call the githubApi for adding labels', async () => { await action.handler(mockContext); expect(mockOctokit.rest.issues.addLabels).toHaveBeenCalledWith({ diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.test.ts index 203014b67b..46d9394606 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.test.ts @@ -24,16 +24,16 @@ import { } from '@backstage/integration'; import { createGithubPagesEnableAction } from './githubPagesEnable'; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; + const mockOctokit = { request: jest.fn(), }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('github:pages', () => { @@ -61,6 +61,7 @@ describe('github:pages', () => { }); beforeEach(() => { + octokitMock.mockImplementation(() => mockOctokit); githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); action = createGithubPagesEnableAction({ @@ -71,6 +72,18 @@ describe('github:pages', () => { afterEach(jest.resetAllMocks); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should work happy path', async () => { await action.handler(mockContext); diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.test.ts index bf340a485e..186d0daeea 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.test.ts @@ -35,6 +35,9 @@ import { entityRefToName } from './gitHelpers'; const publicKey = '2Sg8iYjAxxmI2LvUXpJjkYrMxURPc8r+dB7TJyvvcCU='; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { users: { @@ -62,11 +65,7 @@ const mockOctokit = { request: jest.fn(), }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('github:repo:create', () => { @@ -93,6 +92,7 @@ describe('github:repo:create', () => { }); beforeEach(() => { + octokitMock.mockImplementation(() => mockOctokit); githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); action = createGithubRepoCreateAction({ @@ -110,6 +110,18 @@ describe('github:repo:create', () => { afterEach(jest.resetAllMocks); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should call the githubApis with the correct values for createInOrg', async () => { mockOctokit.rest.users.getByUsername.mockResolvedValue({ data: { type: 'Organization' }, diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.test.ts index 7fdf2e1af6..e845d69954 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.test.ts @@ -52,6 +52,9 @@ const initRepoAndPushMocked = initRepoAndPush as jest.Mock< Promise<{ commitHash: string }> >; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { repos: { @@ -60,11 +63,7 @@ const mockOctokit = { }, }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('github:repo:push', () => { @@ -93,6 +92,8 @@ describe('github:repo:push', () => { beforeEach(() => { jest.resetAllMocks(); + octokitMock.mockImplementation(() => mockOctokit); + initRepoAndPushMocked.mockResolvedValue({ commitHash: 'test123' }); githubCredentialsProvider = @@ -104,6 +105,18 @@ describe('github:repo:push', () => { }); }); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should call initRepoAndPush with the correct values', async () => { mockOctokit.rest.repos.get.mockResolvedValue({ data: { diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.test.ts index 30bdcef20d..cc0fa28ae3 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.test.ts @@ -24,6 +24,9 @@ import { createMockActionContext } from '@backstage/plugin-scaffolder-node-test- import { ConfigReader } from '@backstage/config'; import { TemplateAction } from '@backstage/plugin-scaffolder-node'; +import { Octokit } from 'octokit'; + +const octokitMock = Octokit as unknown as jest.Mock; const mockOctokit = { rest: { repos: { @@ -32,11 +35,7 @@ const mockOctokit = { }, }; jest.mock('octokit', () => ({ - Octokit: class { - constructor() { - return mockOctokit; - } - }, + Octokit: jest.fn(), })); describe('github:repository:webhook:create', () => { @@ -56,6 +55,7 @@ describe('github:repository:webhook:create', () => { beforeEach(() => { jest.resetAllMocks(); + octokitMock.mockImplementation(() => mockOctokit); githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); action = createGithubWebhookAction({ @@ -72,6 +72,18 @@ describe('github:repository:webhook:create', () => { }, }); + it('should pass context logger to Octokit client', async () => { + try { + await action.handler(mockContext); + } catch (e) { + // no-op + } + + expect(octokitMock).toHaveBeenCalledWith( + expect.objectContaining({ log: mockContext.logger }), + ); + }); + it('should call the githubApi for creating repository Webhook', async () => { const repoUrl = 'github.com?repo=repo&owner=owner'; const webhookUrl = 'https://example.com/payload'; From 6579c2c294c56ee7d0f8ef241d3f7f18edf69087 Mon Sep 17 00:00:00 2001 From: Kevin Snyder Date: Thu, 10 Apr 2025 10:43:26 -0700 Subject: [PATCH 3/4] Add changeset Signed-off-by: Kevin Snyder --- .changeset/common-goats-raise.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/common-goats-raise.md diff --git a/.changeset/common-goats-raise.md b/.changeset/common-goats-raise.md new file mode 100644 index 0000000000..c2d797b719 --- /dev/null +++ b/.changeset/common-goats-raise.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-github': patch +--- + +Use action context logger in Octokit client From e9fda1308308cdeaf452d922bb9b0cbc31672548 Mon Sep 17 00:00:00 2001 From: Kevin Snyder Date: Tue, 15 Apr 2025 11:03:30 -0700 Subject: [PATCH 4/4] Remove try-catch blocks from tests Signed-off-by: Kevin Snyder --- .../src/actions/github.test.ts | 12 +++++++----- .../src/actions/githubActionsDispatch.test.ts | 6 +----- .../src/actions/githubAutolinks.test.ts | 6 +----- .../src/actions/githubBranchProtection.test.ts | 6 +----- .../src/actions/githubDeployKey.test.ts | 13 ++++++++----- .../src/actions/githubEnvironment.test.ts | 6 +----- .../src/actions/githubIssuesLabel.test.ts | 6 +----- .../src/actions/githubPagesEnable.test.ts | 6 +----- .../src/actions/githubRepoCreate.test.ts | 12 +++++++----- .../src/actions/githubRepoPush.test.ts | 13 ++++++++----- .../src/actions/githubWebhook.test.ts | 6 +----- 11 files changed, 37 insertions(+), 55 deletions(-) diff --git a/plugins/scaffolder-backend-module-github/src/actions/github.test.ts b/plugins/scaffolder-backend-module-github/src/actions/github.test.ts index 97867226c6..903ec28225 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/github.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/github.test.ts @@ -140,11 +140,13 @@ describe('publish:github', () => { afterEach(jest.resetAllMocks); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + mockOctokit.rest.users.getByUsername.mockResolvedValue({ + data: { type: 'Organization' }, + }); + + mockOctokit.rest.repos.createInOrg.mockResolvedValue({ data: {} }); + + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.test.ts index 6924d909d9..7739259c5d 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubActionsDispatch.test.ts @@ -72,11 +72,7 @@ describe('github:actions:dispatch', () => { }); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.test.ts index 0c63376a15..c378afa17d 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubAutolinks.test.ts @@ -71,11 +71,7 @@ describe('github:autolinks:create', () => { workspacePath, }); - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.test.ts index 5f26b9d298..190743c556 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubBranchProtection.test.ts @@ -73,11 +73,7 @@ describe('github:branch-protection:create', () => { afterEach(jest.resetAllMocks); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.test.ts index e1e7885a15..794ec2cf65 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubDeployKey.test.ts @@ -73,11 +73,14 @@ describe('github:deployKey:create', () => { }); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + mockOctokit.rest.actions.getRepoPublicKey.mockResolvedValue({ + data: { + key: publicKey, + key_id: 'keyid', + }, + }); + + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts index ebef47a06b..6a75bde69f 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts @@ -141,11 +141,7 @@ describe('github:environment:create', () => { afterEach(jest.resetAllMocks); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.test.ts index e792c7d91f..b1e1c6e87c 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubIssuesLabel.test.ts @@ -80,11 +80,7 @@ describe('github:issues:label', () => { }); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.test.ts index 46d9394606..23e3077861 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPagesEnable.test.ts @@ -73,11 +73,7 @@ describe('github:pages', () => { afterEach(jest.resetAllMocks); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.test.ts index 186d0daeea..6f901d1d84 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubRepoCreate.test.ts @@ -111,11 +111,13 @@ describe('github:repo:create', () => { afterEach(jest.resetAllMocks); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + mockOctokit.rest.users.getByUsername.mockResolvedValue({ + data: { type: 'Organization' }, + }); + + mockOctokit.rest.repos.createInOrg.mockResolvedValue({ data: {} }); + + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.test.ts index e845d69954..842f63bef2 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubRepoPush.test.ts @@ -106,11 +106,14 @@ describe('github:repo:push', () => { }); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + mockOctokit.rest.repos.get.mockResolvedValue({ + data: { + clone_url: 'https://github.com/clone/url.git', + html_url: 'https://github.com/html/url', + }, + }); + + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }), diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.test.ts index cc0fa28ae3..a42d251c49 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubWebhook.test.ts @@ -73,11 +73,7 @@ describe('github:repository:webhook:create', () => { }); it('should pass context logger to Octokit client', async () => { - try { - await action.handler(mockContext); - } catch (e) { - // no-op - } + await action.handler(mockContext); expect(octokitMock).toHaveBeenCalledWith( expect.objectContaining({ log: mockContext.logger }),