chore review comments

Signed-off-by: blam <ben@blam.sh>

Signed-off-by: blam <ben@blam.sh>

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,

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({

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<OctokitOptions> => {
+}): Promise<OctokitOptions> {
+  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'],
   };
-};
+}

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,
       });