From 83dd263c3b01566fd7f980ffde34e0fd2dc25e81 Mon Sep 17 00:00:00 2001 From: Nathaniel McAuliffe Date: Tue, 15 Jun 2021 03:03:46 -0400 Subject: [PATCH] fix(scale): Refactor Runner Type and Owner (#871) * fix(scale): Refactor Runner Type and Owner * `environment` should not be optional --- .../runners/src/scale-runners/runners.test.ts | 69 ++++++++----------- .../runners/src/scale-runners/runners.ts | 21 +++--- .../runners/src/scale-runners/scale-down.ts | 2 +- .../src/scale-runners/scale-up.test.ts | 44 ++++++------ .../runners/src/scale-runners/scale-up.ts | 48 ++++++------- 5 files changed, 86 insertions(+), 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 index d3569cc463..0c3cab19c8 100644 --- 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 index d2a5c4b92d..e5e9b3f544 100644 --- 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 { @@ -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 { @@ -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 index c79bbc2b44..7d8a98a4a2 100644 --- 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 { // 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 index f3bc78b633..e2f6640ad7 100644 --- 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}`, }); }); }); diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts index 2cfed5bdb2..dfbbf5d10c 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts @@ -30,16 +30,16 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage const githubClient = await createOctoClient(ghAuth.token, ghesApiUrl); installationId = enableOrgLevel ? ( - await githubClient.apps.getOrgInstallation({ - org: payload.repositoryOwner, - }) - ).data.id + await githubClient.apps.getOrgInstallation({ + org: payload.repositoryOwner, + }) + ).data.id : ( - await githubClient.apps.getRepoInstallation({ - owner: payload.repositoryOwner, - repo: payload.repositoryName, - }) - ).data.id; + await githubClient.apps.getRepoInstallation({ + owner: payload.repositoryOwner, + repo: payload.repositoryName, + }) + ).data.id; } const ghAuth = await createGithubAuth(installationId, 'installation', ghesApiUrl); @@ -51,20 +51,17 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage repo: payload.repositoryName, }); - const repoName = enableOrgLevel ? undefined : `${payload.repositoryOwner}/${payload.repositoryName}`; - const orgName = enableOrgLevel ? payload.repositoryOwner : undefined; + const runnerType = enableOrgLevel ? 'Org' : 'Repo'; + const runnerOwner = enableOrgLevel ? payload.repositoryOwner : `${payload.repositoryOwner}/${payload.repositoryName}`; if (checkRun.data.status === 'queued') { const currentRunners = await listRunners({ - environment: environment, - repoName: repoName, + environment, + runnerType, + runnerOwner }); console.info( - `${ - enableOrgLevel - ? `Organization ${payload.repositoryOwner}` - : `Repo ${payload.repositoryOwner}/${payload.repositoryName}` - } has ${currentRunners.length}/${maximumRunners} runners`, + `${runnerType} ${runnerOwner} has ${currentRunners.length}/${maximumRunners} runners`, ); if (currentRunners.length < maximumRunners) { @@ -72,9 +69,9 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage const registrationToken = enableOrgLevel ? await githubInstallationClient.actions.createRegistrationTokenForOrg({ org: payload.repositoryOwner }) : await githubInstallationClient.actions.createRegistrationTokenForRepo({ - owner: payload.repositoryOwner, - repo: payload.repositoryName, - }); + owner: payload.repositoryOwner, + repo: payload.repositoryName, + }); const token = registrationToken.data.token; const labelsArgument = runnerExtraLabels !== undefined ? `--labels ${runnerExtraLabels}` : ''; @@ -83,11 +80,10 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage await createRunner({ environment: environment, runnerConfig: enableOrgLevel - ? `--url ${configBaseUrl}/${payload.repositoryOwner} --token ${token} ${labelsArgument}${runnerGroupArgument}` - : `--url ${configBaseUrl}/${payload.repositoryOwner}/${payload.repositoryName} ` + - `--token ${token} ${labelsArgument}`, - orgName: orgName, - repoName: repoName, + ? `--url ${configBaseUrl}/${runnerOwner} --token ${token} ${labelsArgument}${runnerGroupArgument}` + : `--url ${configBaseUrl}/${runnerOwner} --token ${token} ${labelsArgument}`, + runnerType, + runnerOwner, }); } else { console.info('No runner will be created, maximum number of runners reached.');