From 5b7079a6965768137fbe801f91bca9bb9e240436 Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 21 Feb 2022 20:48:09 +0100 Subject: [PATCH] chore review comments Signed-off-by: blam Signed-off-by: blam --- .../actions/builtin/github/OctokitProvider.ts | 4 ++-- .../actions/builtin/github/githubWebhook.ts | 8 ++++---- .../actions/builtin/github/helpers.ts | 20 +++++++------------ .../actions/builtin/publish/github.ts | 8 ++++---- 4 files changed, 17 insertions(+), 23 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/OctokitProvider.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/OctokitProvider.ts index 92b8554e37..2f3c43a6b8 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/OctokitProvider.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/OctokitProvider.ts @@ -34,7 +34,7 @@ export type OctokitIntegration = { * OctokitProvider supports GitHub credentials caching out of the box. * * @deprecated we are no longer providing a way from the scaffolder to generate octokit instances. - * Implement your own if you're using this method from an external package, or suse the internal `getOctokitOptions` function instead + * Implement your own if you're using this method from an external package, or use the internal `getOctokitOptions` function instead */ export class OctokitProvider { private readonly integrations: ScmIntegrationRegistry; @@ -56,7 +56,7 @@ export class OctokitProvider { * @param repoUrl - Repository URL * * @deprecated we are no longer providing a way from the scaffolder to generate octokit instances. - * Implement your own if you're using this method from an external package, or suse the internal `getOctokitOptions` function instead + * Implement your own if you're using this method from an external package, or use the internal `getOctokitOptions` function instead */ async getOctokit( repoUrl: string, 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 ac75dd9d82..e2cf744845 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubWebhook.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubWebhook.ts @@ -126,6 +126,10 @@ export function createGithubWebhookAction(options: { ctx.logger.info(`Creating webhook ${webhookUrl} for repo ${repoUrl}`); const { owner, repo } = parseRepoUrl(repoUrl, integrations); + if (!owner) { + throw new InputError('Invalid repository owner provided in repoUrl'); + } + const client = new Octokit( await getOctokitOptions({ integrations, @@ -135,10 +139,6 @@ export function createGithubWebhookAction(options: { }), ); - if (!owner) { - throw new InputError('Invalid repository owner provided in repoUrl'); - } - try { const insecure_ssl = insecureSsl ? '1' : '0'; await client.rest.repos.createWebhook({ 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 4f369b7263..542de013b4 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts @@ -22,18 +22,13 @@ import { import { OctokitOptions } from '@octokit/core/dist-types/types'; import { parseRepoUrl } from '../publish/util'; -interface GetOctokitOptionsInput { +export async function getOctokitOptions(options: { integrations: ScmIntegrationRegistry; credentialsProvider?: GithubCredentialsProvider; token?: string; repoUrl: string; -} -export const getOctokitOptions = async ({ - integrations, - credentialsProvider, - repoUrl, - token, -}: GetOctokitOptionsInput): Promise => { +}): Promise { + const { integrations, credentialsProvider, repoUrl, token } = options; const { owner, repo, host } = parseRepoUrl(repoUrl, integrations); if (!owner) { @@ -46,8 +41,7 @@ export const getOctokitOptions = async ({ throw new InputError(`No integration for host ${host}`); } - // Short circuit the internal Github Token provider the token provided - // by the action or the caller. + // short circuit the `githubCredentialsProvider` if there is a token provided by the caller already if (token) { return { auth: token, @@ -60,8 +54,8 @@ export const getOctokitOptions = async ({ credentialsProvider ?? DefaultGithubCredentialsProvider.fromIntegrations(integrations); - // 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. + // TODO(blam): Consider changing this API to take host and repo instead of repoUrl, as we end up parsing in this function + // and then parsing in the `getCredentials` function too the other side const { token: credentialProviderToken, } = await githubCredentialsProvider.getCredentials({ @@ -81,4 +75,4 @@ export const getOctokitOptions = async ({ baseUrl: integrationConfig.apiBaseUrl, previews: ['nebula-preview'], }; -}; +} 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 7ae9f317cf..37e2ffb7d3 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts @@ -157,6 +157,10 @@ export function createPublishGithubAction(options: { const { owner, repo } = parseRepoUrl(repoUrl, integrations); + if (!owner) { + throw new InputError('Invalid repository owner provided in repoUrl'); + } + const octokitOptions = await getOctokitOptions({ integrations, credentialsProvider: githubCredentialsProvider, @@ -166,10 +170,6 @@ export function createPublishGithubAction(options: { const client = new Octokit(octokitOptions); - if (!owner) { - throw new InputError('Invalid repository owner provided in repoUrl'); - } - const user = await client.rest.users.getByUsername({ username: owner, });