diff --git a/plugins/scaffolder-backend-module-github/src/util.test.ts b/plugins/scaffolder-backend-module-github/src/util.test.ts index 9bbaceee8a..ed215b87d0 100644 --- a/plugins/scaffolder-backend-module-github/src/util.test.ts +++ b/plugins/scaffolder-backend-module-github/src/util.test.ts @@ -35,36 +35,39 @@ const MockOctokit = Octokit as unknown as jest.MockedClass & { }; describe('isRetryEnabled', () => { - it('returns true when both params are undefined', () => { - expect(isRetryEnabled()).toBe(true); - }); - - it('returns true when retries is positive and retryAfter is undefined', () => { - expect(isRetryEnabled(3)).toBe(true); - }); - - it('returns true when retries is undefined and retryAfter is positive', () => { - expect(isRetryEnabled(undefined, 1000)).toBe(true); - }); - - it('returns true when both retries and retryAfter are positive', () => { - expect(isRetryEnabled(5, 2000)).toBe(true); - }); - - it('returns false when retries is 0', () => { - expect(isRetryEnabled(0)).toBe(false); - }); - - it('returns false when retryAfter is 0', () => { - expect(isRetryEnabled(undefined, 0)).toBe(false); - }); - - it('returns false when retries is 0 even if retryAfter is positive', () => { - expect(isRetryEnabled(0, 1000)).toBe(false); - }); - - it('returns false when retryAfter is 0 even if retries is positive', () => { - expect(isRetryEnabled(3, 0)).toBe(false); + it.each([ + { + description: 'returns false when retries is == 0', + options: { retries: 0 }, + expected: false, + }, + { + description: 'returns false when retries is <= 0', + options: { retries: -10 }, + expected: false, + }, + { + description: 'returns false when retryAfter is == 0', + options: { retries: 1, retryAfter: 0 }, + expected: false, + }, + { + description: 'returns false when retryAfter is <= 0', + options: { retries: 1, retryAfter: -10 }, + expected: false, + }, + { + description: 'returns true when retryOptions is undefined', + options: undefined, + expected: true, + }, + { + description: 'returns true when retries is > 0 and retryAfter is > 0', + options: { retries: 5, retryAfter: 5 }, + expected: true, + }, + ])('$description', ({ options, expected }) => { + expect(isRetryEnabled(options)).toBe(expected); }); }); @@ -82,7 +85,8 @@ describe('getOctokitClient', () => { }); it('returns a plain Octokit client when retries is 0', () => { - const client = getOctokitClient(octokitOptions, logger, 0); + const retryOptions = { retries: 0 }; + const client = getOctokitClient(octokitOptions, logger, retryOptions); expect(MockOctokit.plugin).not.toHaveBeenCalled(); expect(MockOctokit).toHaveBeenCalledWith({ @@ -93,7 +97,8 @@ describe('getOctokitClient', () => { }); it('returns a plain Octokit client when retryAfter is 0', () => { - const client = getOctokitClient(octokitOptions, logger, 3, 0); + const retryOptions = { retries: 3, retryAfter: 0 }; + const client = getOctokitClient(octokitOptions, logger, retryOptions); expect(MockOctokit.plugin).not.toHaveBeenCalled(); expect(MockOctokit).toHaveBeenCalledWith({ @@ -103,7 +108,7 @@ describe('getOctokitClient', () => { expect(client).toBe(mockOctokitInstance); }); - it('returns a retry-enabled Octokit client with default retries and delay', () => { + it('returns a retry-enabled Octokit client with default retries and delay when retryOptions is undefined', () => { const client = getOctokitClient(octokitOptions, logger); expect(MockOctokit.plugin).toHaveBeenCalledWith(retry); @@ -119,7 +124,8 @@ describe('getOctokitClient', () => { }); it('returns a retry-enabled Octokit client with custom retries and delay', () => { - const client = getOctokitClient(octokitOptions, logger, 5, 2000); + const retryOptions = { retries: 5, retryAfter: 2000 }; + const client = getOctokitClient(octokitOptions, logger, retryOptions); expect(MockOctokit.plugin).toHaveBeenCalledWith(retry); expect(MockOctokit).toHaveBeenCalledWith({ @@ -139,7 +145,8 @@ describe('getOctokitClient', () => { request: { timeout: 60_000 }, }; - getOctokitClient(optionsWithRequest, logger, 3, 1000); + const retryOptions = { retries: 3, retryAfter: 1000 }; + getOctokitClient(optionsWithRequest, logger, retryOptions); expect(MockOctokit).toHaveBeenCalledWith({ ...optionsWithRequest, diff --git a/plugins/scaffolder-backend-module-github/src/util.ts b/plugins/scaffolder-backend-module-github/src/util.ts index 6b9769e675..e4965bff5b 100644 --- a/plugins/scaffolder-backend-module-github/src/util.ts +++ b/plugins/scaffolder-backend-module-github/src/util.ts @@ -33,26 +33,33 @@ const DEFAULT_TIMEOUT_MS = 60_000; const DEFAULT_RETRY_ATTEMPTS = 3; const DEFAULT_RETRY_DELAY_MS = 1000; +type RetryOptions = { + // The number of retry attempts for failed requests + retries?: number; + // The delay in milliseconds between retry attempts + retryAfter?: number; +}; + /** * Helper function to determine if retries are enabled based on the provided * options. * - * Retries are enabled by default, but can be disabled by setting either `retries` - * or `retryAfter` to 0. + * @param retryOptions - Optional retry configuration options, including the number + * of retries and the delay between retries. * - * @param retries - The number of retry attempts for failed requests. Default is 3. - * Setting to 0 will disable retries. - * @param retryAfter - The delay in milliseconds between retry attempts. - * Default is 1000ms. Setting to 0 will disable retries. - * - * @returns A boolean indicating whether retries are enabled or not. + * @returns False if retries/retryAfter are explicitly set to 0 or less. + * Returns true otherwise. */ -export function isRetryEnabled(retries?: number, retryAfter?: number): boolean { - if (retries === 0) { +export function isRetryEnabled(retryOptions?: RetryOptions): boolean { + if (retryOptions === undefined) { + return true; + } + + if (retryOptions.retries !== undefined && retryOptions.retries <= 0) { return false; } - if (retryAfter === 0) { + if (retryOptions.retryAfter !== undefined && retryOptions.retryAfter <= 0) { return false; } @@ -72,22 +79,20 @@ export function isRetryEnabled(retries?: number, retryAfter?: number): boolean { * Generally provided by the `getOctokitOptions` helper. * @param logger - LoggerService instance for logging retry attempts and * failures. - * @param retries - The number of retry attempts for failed requests. - * Default is 3. Setting to 0 will disable retries. - * @param retryAfter - The delay in milliseconds between retry attempts. - * Default is 1000ms. Setting to 0 will disable retries. + * @param retryOptions - Optional retry configuration options, including the + * number of retries and the delay between retries. + * * @returns An authenticated Octokit client instance based on the provided * options. */ export function getOctokitClient( octokitOptions: OctokitOptions, logger: LoggerService, - retries: number = DEFAULT_RETRY_ATTEMPTS, - retryAfter: number = DEFAULT_RETRY_DELAY_MS, + retryOptions?: RetryOptions, ): Octokit { // Default behavior is to enable retries, but allow callers to disable by // explicitly setting retries or retryAfter to 0 - if (!isRetryEnabled(retries, retryAfter)) { + if (!isRetryEnabled(retryOptions)) { return new Octokit({ ...octokitOptions, log: logger, @@ -96,6 +101,16 @@ export function getOctokitClient( // Update the octokit options to include retry configuration with logging const OctokitClient = Octokit.plugin(retry); + + // From the octokit/plugin-retry documentation, specifying these values will + // always retry regardless of the response code + const retries = retryOptions?.retries + ? retryOptions?.retries + : DEFAULT_RETRY_ATTEMPTS; + const retryAfter = retryOptions?.retryAfter + ? retryOptions?.retryAfter + : DEFAULT_RETRY_DELAY_MS; + return new OctokitClient({ ...octokitOptions, request: {