chore: switch retryOptions to an options object

Signed-off-by: Jan Michael Ong <adobejmong@gmail.com>

plugins/scaffolder-backend-module-github/src/util.test.ts

@@ -35,36 +35,39 @@ const MockOctokit = Octokit as unknown as jest.MockedClass<typeof Octokit> & {
 };
 
 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,

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