From 2b69f8a4fd4d10c21d50e88d5f611e61251d06bd Mon Sep 17 00:00:00 2001 From: "@pawelmitka" Date: Tue, 31 Aug 2021 11:43:58 +0200 Subject: [PATCH] refactor: replace `getOctokit` with `OctokitProvider.getOctokit` to allow credentails caching Signed-off-by: @pawelmitka --- .../builtin/github/githubActionsDispatch.ts | 8 +- .../actions/builtin/github/githubWebhook.ts | 8 +- .../actions/builtin/github/helpers.test.ts | 30 ++---- .../actions/builtin/github/helpers.ts | 100 ++++++++++-------- .../actions/builtin/publish/github.ts | 8 +- 5 files changed, 72 insertions(+), 82 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.ts index c2d750051e..c7d2919efb 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.ts @@ -15,12 +15,13 @@ */ import { ScmIntegrationRegistry } from '@backstage/integration'; import { createTemplateAction } from '../../createTemplateAction'; -import { getOctokit } from './helpers'; +import { OctokitProvider } from './helpers'; export function createGithubActionsDispatchAction(options: { integrations: ScmIntegrationRegistry; }) { const { integrations } = options; + const octokitProvider = new OctokitProvider(integrations); return createTemplateAction<{ repoUrl: string; @@ -61,10 +62,7 @@ export function createGithubActionsDispatchAction(options: { `Dispatching workflow ${workflowId} for repo ${repoUrl} on ${branchOrTagName}`, ); - const { client, owner, repo } = await getOctokit({ - integrations, - repoUrl, - }); + const { client, owner, repo } = await octokitProvider.getOctokit(repoUrl); await client.rest.actions.createWorkflowDispatch({ owner, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubWebhook.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubWebhook.ts index 5b83eca3fd..eabfebeb28 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubWebhook.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubWebhook.ts @@ -15,7 +15,7 @@ */ import { ScmIntegrationRegistry } from '@backstage/integration'; import { createTemplateAction } from '../../createTemplateAction'; -import { getOctokit } from './helpers'; +import { OctokitProvider } from './helpers'; type ContentType = 'form' | 'json'; @@ -23,6 +23,7 @@ export function createGithubWebhookAction(options: { integrations: ScmIntegrationRegistry; }) { const { integrations } = options; + const octokitProvider = new OctokitProvider(integrations); return createTemplateAction<{ repoUrl: string; @@ -96,10 +97,7 @@ export function createGithubWebhookAction(options: { ctx.logger.info(`Creating webhook ${webhookUrl} for repo ${repoUrl}`); - const { client, owner, repo } = await getOctokit({ - integrations, - repoUrl, - }); + const { client, owner, repo } = await octokitProvider.getOctokit(repoUrl); try { const insecure_ssl = insecureSsl ? '1' : '0'; diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.test.ts index e16f76d89f..a72a775701 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { getOctokit } from './helpers'; +import { OctokitProvider } from './helpers'; import { ScmIntegrations } from '@backstage/integration'; import { ConfigReader } from '@backstage/config'; @@ -29,6 +29,7 @@ describe('getOctokit', () => { }); const integrations = ScmIntegrations.fromConfig(config); + const octokitProvider = new OctokitProvider(integrations); beforeEach(() => { jest.resetAllMocks(); @@ -36,43 +37,30 @@ describe('getOctokit', () => { it('should throw an error when the repoUrl is not well formed', async () => { await expect( - getOctokit({ - integrations, - repoUrl: 'github.com?repo=bob', - }), + octokitProvider.getOctokit('github.com?repo=bob'), ).rejects.toThrow(/missing owner/); await expect( - getOctokit({ - integrations, - repoUrl: 'github.com?owner=owner', - }), + octokitProvider.getOctokit('github.com?owner=owner'), ).rejects.toThrow(/missing repo/); }); it('should throw if there is no integration config provided', async () => { await expect( - getOctokit({ - integrations, - repoUrl: 'missing.com?repo=bob&owner=owner', - }), + octokitProvider.getOctokit('missing.com?repo=bob&owner=owner'), ).rejects.toThrow(/No matching integration configuration/); }); it('should throw if there is no token in the integration config that is returned', async () => { await expect( - getOctokit({ - integrations, - repoUrl: 'ghe.github.com?repo=bob&owner=owner', - }), + octokitProvider.getOctokit('ghe.github.com?repo=bob&owner=owner'), ).rejects.toThrow(/No token available for host/); }); it('should return proper Octokit', async () => { - const { client, token, owner, repo } = await getOctokit({ - integrations, - repoUrl: 'github.com?repo=bob&owner=owner', - }); + const { client, token, owner, repo } = await octokitProvider.getOctokit( + 'github.com?repo=bob&owner=owner', + ); expect(client).toBeDefined(); expect(token).toBe('tokenlols'); expect(owner).toBe('owner'); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts index 89c7b43a1a..4ed10b9d64 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts @@ -22,62 +22,68 @@ import { import { Octokit } from '@octokit/rest'; import { parseRepoUrl } from '../publish/util'; -type OctokitOptions = { - integrations: ScmIntegrationRegistry; - repoUrl: string; -}; - -type OctokitIntegration = { +export type OctokitIntegration = { client: Octokit; token: string; owner: string; repo: string; }; -export const getOctokit = async ({ - integrations, - repoUrl, -}: OctokitOptions): Promise => { - const { owner, repo, host } = parseRepoUrl(repoUrl, integrations); +export class OctokitProvider { + private readonly integrations: ScmIntegrationRegistry; + private readonly credentialsProviders: Map; - if (!owner) { - throw new InputError(`No owner provided for repo ${repoUrl}`); - } - - const integrationConfig = integrations.github.byHost(host)?.config; - - if (!integrationConfig) { - throw new InputError(`No integration for host ${host}`); - } - - const credentialsProvider = - GithubCredentialsProvider.create(integrationConfig); - - if (!credentialsProvider) { - throw new InputError( - `No matching credentials for host ${host}, please check your integrations config`, + constructor(integrations: ScmIntegrationRegistry) { + this.integrations = integrations; + this.credentialsProviders = new Map( + integrations.github.list().map(integration => { + const provider = GithubCredentialsProvider.create(integration.config); + return [integration.config.host, provider]; + }), ); } - // TODO(blam): Consider changing this API to have owner, repo interface instead of URL as the it's - // needless to create URL and then parse again the other side. - const { token } = await credentialsProvider.getCredentials({ - url: `https://${host}/${encodeURIComponent(owner)}/${encodeURIComponent( - repo, - )}`, - }); + async getOctokit(repoUrl: string): Promise { + const { owner, repo, host } = parseRepoUrl(repoUrl, this.integrations); - if (!token) { - throw new InputError( - `No token available for host: ${host}, with owner ${owner}, and repo ${repo}`, - ); + if (!owner) { + throw new InputError(`No owner provided for repo ${repoUrl}`); + } + + const integrationConfig = this.integrations.github.byHost(host)?.config; + + if (!integrationConfig) { + throw new InputError(`No integration for host ${host}`); + } + + const credentialsProvider = this.credentialsProviders.get(host); + + if (!credentialsProvider) { + throw new InputError( + `No matching credentials for host ${host}, please check your integrations config`, + ); + } + + // TODO(blam): Consider changing this API to have owner, repo interface instead of URL as the it's + // needless to create URL and then parse again the other side. + const { token } = await credentialsProvider.getCredentials({ + url: `https://${host}/${encodeURIComponent(owner)}/${encodeURIComponent( + repo, + )}`, + }); + + if (!token) { + throw new InputError( + `No token available for host: ${host}, with owner ${owner}, and repo ${repo}`, + ); + } + + const client = new Octokit({ + auth: token, + baseUrl: integrationConfig.apiBaseUrl, + previews: ['nebula-preview'], + }); + + return { client, token, owner, repo }; } - - const client = new Octokit({ - auth: token, - baseUrl: integrationConfig.apiBaseUrl, - previews: ['nebula-preview'], - }); - - return { client, token, owner, repo }; -}; +} diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts index 857847825a..eaedbf275f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts @@ -21,7 +21,7 @@ import { import { getRepoSourceDirectory } from './util'; import { createTemplateAction } from '../../createTemplateAction'; import { Config } from '@backstage/config'; -import { getOctokit } from '../github/helpers'; +import { OctokitProvider } from '../github/helpers'; type Permission = 'pull' | 'push' | 'admin' | 'maintain' | 'triage'; type Collaborator = { access: Permission; username: string }; @@ -31,6 +31,7 @@ export function createPublishGithubAction(options: { config: Config; }) { const { integrations, config } = options; + const octokitProvider = new OctokitProvider(integrations); return createTemplateAction<{ repoUrl: string; @@ -140,10 +141,9 @@ export function createPublishGithubAction(options: { topics, } = ctx.input; - const { client, token, owner, repo } = await getOctokit({ - integrations, + const { client, token, owner, repo } = await octokitProvider.getOctokit( repoUrl, - }); + ); const user = await client.users.getByUsername({ username: owner,