fix(scale): Refactor Runner Type and Owner (#871)

* fix(scale): Refactor Runner Type and Owner * `environment` should not be optional
philips-labs · Jun 17, 2021 · 83dd263 · 83dd263
1 parent ff2791a
commit 83dd263
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 98 deletions.
diff --git a/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts b/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts
@@ -1,5 +1,4 @@
-import { listRunners, RunnerInfo, createRunner } from './runners';
-import { EC2, SSM } from 'aws-sdk';
+import { listRunners, createRunner } from './runners';
 
 const mockEC2 = { describeInstances: jest.fn(), runInstances: jest.fn() };
 const mockSSM = { putParameter: jest.fn() };
@@ -8,6 +7,10 @@ jest.mock('aws-sdk', () => ({
   SSM: jest.fn().mockImplementation(() => mockSSM),
 }));
 
+const ORG_NAME = 'SomeAwesomeCoder';
+const REPO_NAME = `${ORG_NAME}/some-amazing-library`;
+const ENVIRONMENT = 'unit-test-environment';
+
 describe('list instances', () => {
   const mockDescribeInstances = { promise: jest.fn() };
   beforeEach(() => {
@@ -30,8 +33,8 @@ describe('list instances', () => {
               LaunchTime: new Date('2020-10-11T14:48:00.000+09:00'),
               InstanceId: 'i-5678',
               Tags: [
-                { Key: 'Repo', Value: 'SomeAwesomeCoder/some-amazing-library' },
-                { Key: 'Org', Value: 'SomeAwesomeCoder' },
+                { Key: 'Repo', Value: REPO_NAME },
+                { Key: 'Org', Value: ORG_NAME },
                 { Key: 'Application', Value: 'github-action-runner' },
               ],
             },
@@ -54,8 +57,8 @@ describe('list instances', () => {
     expect(resp).toContainEqual({
       instanceId: 'i-5678',
       launchTime: new Date('2020-10-11T14:48:00.000+09:00'),
-      repo: 'SomeAwesomeCoder/some-amazing-library',
-      org: 'SomeAwesomeCoder',
+      repo: REPO_NAME,
+      org: ORG_NAME,
     });
   });
 
@@ -65,46 +68,34 @@ describe('list instances', () => {
   });
 
   it('filters instances on repo name', async () => {
-    await listRunners({ repoName: 'SomeAwesomeCoder/some-amazing-library' });
+    await listRunners({ runnerType: 'Repo', runnerOwner: REPO_NAME, environment: undefined });
     expect(mockEC2.describeInstances).toBeCalledWith({
       Filters: [
         { Name: 'tag:Application', Values: ['github-action-runner'] },
         { Name: 'instance-state-name', Values: ['running', 'pending'] },
-        { Name: 'tag:Repo', Values: ['SomeAwesomeCoder/some-amazing-library'] },
+        { Name: 'tag:Repo', Values: [REPO_NAME] },
       ],
     });
   });
 
   it('filters instances on org name', async () => {
-    await listRunners({ orgName: 'SomeAwesomeCoder' });
+    await listRunners({ runnerType: 'Org', runnerOwner: ORG_NAME, environment: undefined });
     expect(mockEC2.describeInstances).toBeCalledWith({
       Filters: [
         { Name: 'tag:Application', Values: ['github-action-runner'] },
         { Name: 'instance-state-name', Values: ['running', 'pending'] },
-        { Name: 'tag:Org', Values: ['SomeAwesomeCoder'] },
+        { Name: 'tag:Org', Values: [ORG_NAME] },
       ],
     });
   });
 
   it('filters instances on org name', async () => {
-    await listRunners({ environment: 'unit-test-environment' });
-    expect(mockEC2.describeInstances).toBeCalledWith({
-      Filters: [
-        { Name: 'tag:Application', Values: ['github-action-runner'] },
-        { Name: 'instance-state-name', Values: ['running', 'pending'] },
-        { Name: 'tag:Environment', Values: ['unit-test-environment'] },
-      ],
-    });
-  });
-
-  it('filters instances on both org name and repo name', async () => {
-    await listRunners({ orgName: 'SomeAwesomeCoder', repoName: 'SomeAwesomeCoder/some-amazing-library' });
+    await listRunners({ environment: ENVIRONMENT });
     expect(mockEC2.describeInstances).toBeCalledWith({
       Filters: [
         { Name: 'tag:Application', Values: ['github-action-runner'] },
         { Name: 'instance-state-name', Values: ['running', 'pending'] },
-        { Name: 'tag:Repo', Values: ['SomeAwesomeCoder/some-amazing-library'] },
-        { Name: 'tag:Org', Values: ['SomeAwesomeCoder'] },
+        { Name: 'tag:Environment', Values: [ENVIRONMENT] },
       ],
     });
   });
@@ -132,9 +123,9 @@ describe('create runner', () => {
   it('calls run instances with the correct config for repo', async () => {
     await createRunner({
       runnerConfig: 'bla',
-      environment: 'unit-test-env',
-      repoName: 'SomeAwesomeCoder/some-amazing-library',
-      orgName: undefined,
+      environment: ENVIRONMENT,
+      runnerType: 'Repo',
+      runnerOwner: REPO_NAME
     });
     expect(mockEC2.runInstances).toBeCalledWith({
       MaxCount: 1,
@@ -146,7 +137,7 @@ describe('create runner', () => {
           ResourceType: 'instance',
           Tags: [
             { Key: 'Application', Value: 'github-action-runner' },
-            { Key: 'Repo', Value: 'SomeAwesomeCoder/some-amazing-library' },
+            { Key: 'Repo', Value: REPO_NAME },
           ],
         },
       ],
@@ -156,9 +147,9 @@ describe('create runner', () => {
   it('calls run instances with the correct config for org', async () => {
     await createRunner({
       runnerConfig: 'bla',
-      environment: 'unit-test-env',
-      repoName: undefined,
-      orgName: 'SomeAwesomeCoder',
+      environment: ENVIRONMENT,
+      runnerType: 'Org',
+      runnerOwner: ORG_NAME,
     });
     expect(mockEC2.runInstances).toBeCalledWith({
       MaxCount: 1,
@@ -170,7 +161,7 @@ describe('create runner', () => {
           ResourceType: 'instance',
           Tags: [
             { Key: 'Application', Value: 'github-action-runner' },
-            { Key: 'Org', Value: 'SomeAwesomeCoder' },
+            { Key: 'Org', Value: ORG_NAME },
           ],
         },
       ],
@@ -180,12 +171,12 @@ describe('create runner', () => {
   it('creates ssm parameters for each created instance', async () => {
     await createRunner({
       runnerConfig: 'bla',
-      environment: 'unit-test-env',
-      repoName: undefined,
-      orgName: 'SomeAwesomeCoder',
+      environment: ENVIRONMENT,
+      runnerType: 'Org',
+      runnerOwner: ORG_NAME,
     });
     expect(mockSSM.putParameter).toBeCalledWith({
-      Name: 'unit-test-env-i-1234',
+      Name: `${ENVIRONMENT}-i-1234`,
       Value: 'bla',
       Type: 'SecureString',
     });
@@ -197,9 +188,9 @@ describe('create runner', () => {
     });
     await createRunner({
       runnerConfig: 'bla',
-      environment: 'unit-test-env',
-      repoName: undefined,
-      orgName: 'SomeAwesomeCoder',
+      environment: ENVIRONMENT,
+      runnerType: 'Org',
+      runnerOwner: ORG_NAME,
     });
     expect(mockSSM.putParameter).not.toBeCalled();
   });

diff --git a/modules/runners/lambdas/runners/src/scale-runners/runners.ts b/modules/runners/lambdas/runners/src/scale-runners/runners.ts
@@ -8,9 +8,9 @@ export interface RunnerInfo {
 }
 
 export interface ListRunnerFilters {
-  repoName?: string;
-  orgName?: string;
-  environment?: string;
+  runnerType?: 'Org' | 'Repo';
+  runnerOwner?: string;
+  environment: string | undefined;
 }
 
 export async function listRunners(filters: ListRunnerFilters | undefined = undefined): Promise<RunnerInfo[]> {
@@ -23,11 +23,8 @@ export async function listRunners(filters: ListRunnerFilters | undefined = undef
     if (filters.environment !== undefined) {
       ec2Filters.push({ Name: 'tag:Environment', Values: [filters.environment] });
     }
-    if (filters.repoName !== undefined) {
-      ec2Filters.push({ Name: 'tag:Repo', Values: [filters.repoName] });
-    }
-    if (filters.orgName !== undefined) {
-      ec2Filters.push({ Name: 'tag:Org', Values: [filters.orgName] });
+    if (filters.runnerType && filters.runnerOwner) {
+      ec2Filters.push({ Name: `tag:${filters.runnerType}`, Values: [filters.runnerOwner] });
     }
   }
   const runningInstances = await ec2.describeInstances({ Filters: ec2Filters }).promise();
@@ -52,8 +49,8 @@ export async function listRunners(filters: ListRunnerFilters | undefined = undef
 export interface RunnerInputParameters {
   runnerConfig: string;
   environment: string;
-  repoName?: string;
-  orgName?: string;
+  runnerType: 'Org' | 'Repo';
+  runnerOwner: string;
 }
 
 export async function terminateRunner(runner: RunnerInfo): Promise<void> {
@@ -89,8 +86,8 @@ export async function createRunner(runnerParameters: RunnerInputParameters): Pro
           Tags: [
             { Key: 'Application', Value: 'github-action-runner' },
             {
-              Key: runnerParameters.orgName ? 'Org' : 'Repo',
-              Value: runnerParameters.orgName ? runnerParameters.orgName : runnerParameters.repoName,
+              Key: runnerParameters.runnerType,
+              Value: runnerParameters.runnerOwner
             },
           ],
         },

diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts
@@ -139,7 +139,7 @@ export async function scaleDown(): Promise<void> {
   // list and sort runners, newest first. This ensure we keep the newest runners longer.
   const runners = (
     await listRunners({
-      environment: environment,
+      environment
     })
   ).sort((a, b): number => {
     if (a.launchTime === undefined) return 1;

diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts
@@ -126,7 +126,8 @@ describe('scaleUp with GHES', () => {
       await scaleUp('aws:sqs', TEST_DATA);
       expect(listRunners).toBeCalledWith({
         environment: 'unit-test-environment',
-        repoName: undefined,
+        runnerType: 'Org',
+        runnerOwner: TEST_DATA.repositoryOwner
       });
     });
 
@@ -174,8 +175,8 @@ describe('scaleUp with GHES', () => {
       expect(createRunner).toBeCalledWith({
         environment: 'unit-test-environment',
         runnerConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} --token 1234abcd `,
-        orgName: TEST_DATA.repositoryOwner,
-        repoName: undefined,
+        runnerType: 'Org',
+        runnerOwner: TEST_DATA.repositoryOwner,
       });
     });
 
@@ -187,8 +188,8 @@ describe('scaleUp with GHES', () => {
         environment: 'unit-test-environment',
         runnerConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} ` +
           `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`,
-        orgName: TEST_DATA.repositoryOwner,
-        repoName: undefined,
+        runnerType: 'Org',
+        runnerOwner: TEST_DATA.repositoryOwner,
       });
     });
   });
@@ -202,7 +203,8 @@ describe('scaleUp with GHES', () => {
       await scaleUp('aws:sqs', TEST_DATA);
       expect(listRunners).toBeCalledWith({
         environment: 'unit-test-environment',
-        repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
+        runnerType: 'Repo',
+        runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
       });
     });
 
@@ -253,8 +255,8 @@ describe('scaleUp with GHES', () => {
         runnerConfig: `--url ` +
           `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` +
           `--token 1234abcd --labels label1,label2`,
-        orgName: undefined,
-        repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
+        runnerType: 'Repo',
+        runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
       });
     });
 
@@ -267,8 +269,8 @@ describe('scaleUp with GHES', () => {
         runnerConfig: `--url ` +
           `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` +
           `--token 1234abcd --labels label1,label2`,
-        orgName: undefined,
-        repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
+        runnerType: 'Repo',
+        runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
       });
     });
   });
@@ -322,7 +324,8 @@ describe('scaleUp with public GH', () => {
       await scaleUp('aws:sqs', TEST_DATA);
       expect(listRunners).toBeCalledWith({
         environment: 'unit-test-environment',
-        repoName: undefined,
+        runnerType: 'Org',
+        runnerOwner: TEST_DATA.repositoryOwner
       });
     });
 
@@ -345,8 +348,8 @@ describe('scaleUp with public GH', () => {
       expect(createRunner).toBeCalledWith({
         environment: 'unit-test-environment',
         runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} --token 1234abcd `,
-        orgName: TEST_DATA.repositoryOwner,
-        repoName: undefined,
+        runnerType: 'Org',
+        runnerOwner: TEST_DATA.repositoryOwner
       });
     });
 
@@ -358,8 +361,8 @@ describe('scaleUp with public GH', () => {
         environment: 'unit-test-environment',
         runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} ` +
           `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`,
-        orgName: TEST_DATA.repositoryOwner,
-        repoName: undefined,
+        runnerType: 'Org',
+        runnerOwner: TEST_DATA.repositoryOwner
       });
     });
   });
@@ -373,7 +376,8 @@ describe('scaleUp with public GH', () => {
       await scaleUp('aws:sqs', TEST_DATA);
       expect(listRunners).toBeCalledWith({
         environment: 'unit-test-environment',
-        repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
+        runnerType: 'Repo',
+        runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
       });
     });
 
@@ -414,8 +418,8 @@ describe('scaleUp with public GH', () => {
         environment: 'unit-test-environment',
         runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` +
           `--token 1234abcd --labels label1,label2`,
-        orgName: undefined,
-        repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
+        runnerType: 'Repo',
+        runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
       });
     });
 
@@ -427,8 +431,8 @@ describe('scaleUp with public GH', () => {
         environment: 'unit-test-environment',
         runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` +
           `--token 1234abcd --labels label1,label2`,
-        orgName: undefined,
-        repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
+        runnerType: 'Repo',
+        runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
       });
     });
   });