Skip to content
This repository was archived by the owner on Jan 16, 2025. It is now read-only.

Commit 83dd263

Browse files
mcaulifnnpalm
mcaulifn
authored and
npalm
committed
fix(scale): Refactor Runner Type and Owner (#871)
* fix(scale): Refactor Runner Type and Owner * `environment` should not be optional
1 parent ff2791a commit 83dd263

File tree

5 files changed

+86
-98
lines changed

5 files changed

+86
-98
lines changed

Diff for: ‎modules/runners/lambdas/runners/src/scale-runners/runners.test.ts

+30-39
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
import { listRunners, RunnerInfo, createRunner } from './runners';
2-
import { EC2, SSM } from 'aws-sdk';
1+
import { listRunners, createRunner } from './runners';
32

43
const mockEC2 = { describeInstances: jest.fn(), runInstances: jest.fn() };
54
const mockSSM = { putParameter: jest.fn() };
@@ -8,6 +7,10 @@ jest.mock('aws-sdk', () => ({
87
SSM: jest.fn().mockImplementation(() => mockSSM),
98
}));
109

10+
const ORG_NAME = 'SomeAwesomeCoder';
11+
const REPO_NAME = `${ORG_NAME}/some-amazing-library`;
12+
const ENVIRONMENT = 'unit-test-environment';
13+
1114
describe('list instances', () => {
1215
const mockDescribeInstances = { promise: jest.fn() };
1316
beforeEach(() => {
@@ -30,8 +33,8 @@ describe('list instances', () => {
3033
LaunchTime: new Date('2020-10-11T14:48:00.000+09:00'),
3134
InstanceId: 'i-5678',
3235
Tags: [
33-
{ Key: 'Repo', Value: 'SomeAwesomeCoder/some-amazing-library' },
34-
{ Key: 'Org', Value: 'SomeAwesomeCoder' },
36+
{ Key: 'Repo', Value: REPO_NAME },
37+
{ Key: 'Org', Value: ORG_NAME },
3538
{ Key: 'Application', Value: 'github-action-runner' },
3639
],
3740
},
@@ -54,8 +57,8 @@ describe('list instances', () => {
5457
expect(resp).toContainEqual({
5558
instanceId: 'i-5678',
5659
launchTime: new Date('2020-10-11T14:48:00.000+09:00'),
57-
repo: 'SomeAwesomeCoder/some-amazing-library',
58-
org: 'SomeAwesomeCoder',
60+
repo: REPO_NAME,
61+
org: ORG_NAME,
5962
});
6063
});
6164

@@ -65,46 +68,34 @@ describe('list instances', () => {
6568
});
6669

6770
it('filters instances on repo name', async () => {
68-
await listRunners({ repoName: 'SomeAwesomeCoder/some-amazing-library' });
71+
await listRunners({ runnerType: 'Repo', runnerOwner: REPO_NAME, environment: undefined });
6972
expect(mockEC2.describeInstances).toBeCalledWith({
7073
Filters: [
7174
{ Name: 'tag:Application', Values: ['github-action-runner'] },
7275
{ Name: 'instance-state-name', Values: ['running', 'pending'] },
73-
{ Name: 'tag:Repo', Values: ['SomeAwesomeCoder/some-amazing-library'] },
76+
{ Name: 'tag:Repo', Values: [REPO_NAME] },
7477
],
7578
});
7679
});
7780

7881
it('filters instances on org name', async () => {
79-
await listRunners({ orgName: 'SomeAwesomeCoder' });
82+
await listRunners({ runnerType: 'Org', runnerOwner: ORG_NAME, environment: undefined });
8083
expect(mockEC2.describeInstances).toBeCalledWith({
8184
Filters: [
8285
{ Name: 'tag:Application', Values: ['github-action-runner'] },
8386
{ Name: 'instance-state-name', Values: ['running', 'pending'] },
84-
{ Name: 'tag:Org', Values: ['SomeAwesomeCoder'] },
87+
{ Name: 'tag:Org', Values: [ORG_NAME] },
8588
],
8689
});
8790
});
8891

8992
it('filters instances on org name', async () => {
90-
await listRunners({ environment: 'unit-test-environment' });
91-
expect(mockEC2.describeInstances).toBeCalledWith({
92-
Filters: [
93-
{ Name: 'tag:Application', Values: ['github-action-runner'] },
94-
{ Name: 'instance-state-name', Values: ['running', 'pending'] },
95-
{ Name: 'tag:Environment', Values: ['unit-test-environment'] },
96-
],
97-
});
98-
});
99-
100-
it('filters instances on both org name and repo name', async () => {
101-
await listRunners({ orgName: 'SomeAwesomeCoder', repoName: 'SomeAwesomeCoder/some-amazing-library' });
93+
await listRunners({ environment: ENVIRONMENT });
10294
expect(mockEC2.describeInstances).toBeCalledWith({
10395
Filters: [
10496
{ Name: 'tag:Application', Values: ['github-action-runner'] },
10597
{ Name: 'instance-state-name', Values: ['running', 'pending'] },
106-
{ Name: 'tag:Repo', Values: ['SomeAwesomeCoder/some-amazing-library'] },
107-
{ Name: 'tag:Org', Values: ['SomeAwesomeCoder'] },
98+
{ Name: 'tag:Environment', Values: [ENVIRONMENT] },
10899
],
109100
});
110101
});
@@ -132,9 +123,9 @@ describe('create runner', () => {
132123
it('calls run instances with the correct config for repo', async () => {
133124
await createRunner({
134125
runnerConfig: 'bla',
135-
environment: 'unit-test-env',
136-
repoName: 'SomeAwesomeCoder/some-amazing-library',
137-
orgName: undefined,
126+
environment: ENVIRONMENT,
127+
runnerType: 'Repo',
128+
runnerOwner: REPO_NAME
138129
});
139130
expect(mockEC2.runInstances).toBeCalledWith({
140131
MaxCount: 1,
@@ -146,7 +137,7 @@ describe('create runner', () => {
146137
ResourceType: 'instance',
147138
Tags: [
148139
{ Key: 'Application', Value: 'github-action-runner' },
149-
{ Key: 'Repo', Value: 'SomeAwesomeCoder/some-amazing-library' },
140+
{ Key: 'Repo', Value: REPO_NAME },
150141
],
151142
},
152143
],
@@ -156,9 +147,9 @@ describe('create runner', () => {
156147
it('calls run instances with the correct config for org', async () => {
157148
await createRunner({
158149
runnerConfig: 'bla',
159-
environment: 'unit-test-env',
160-
repoName: undefined,
161-
orgName: 'SomeAwesomeCoder',
150+
environment: ENVIRONMENT,
151+
runnerType: 'Org',
152+
runnerOwner: ORG_NAME,
162153
});
163154
expect(mockEC2.runInstances).toBeCalledWith({
164155
MaxCount: 1,
@@ -170,7 +161,7 @@ describe('create runner', () => {
170161
ResourceType: 'instance',
171162
Tags: [
172163
{ Key: 'Application', Value: 'github-action-runner' },
173-
{ Key: 'Org', Value: 'SomeAwesomeCoder' },
164+
{ Key: 'Org', Value: ORG_NAME },
174165
],
175166
},
176167
],
@@ -180,12 +171,12 @@ describe('create runner', () => {
180171
it('creates ssm parameters for each created instance', async () => {
181172
await createRunner({
182173
runnerConfig: 'bla',
183-
environment: 'unit-test-env',
184-
repoName: undefined,
185-
orgName: 'SomeAwesomeCoder',
174+
environment: ENVIRONMENT,
175+
runnerType: 'Org',
176+
runnerOwner: ORG_NAME,
186177
});
187178
expect(mockSSM.putParameter).toBeCalledWith({
188-
Name: 'unit-test-env-i-1234',
179+
Name: `${ENVIRONMENT}-i-1234`,
189180
Value: 'bla',
190181
Type: 'SecureString',
191182
});
@@ -197,9 +188,9 @@ describe('create runner', () => {
197188
});
198189
await createRunner({
199190
runnerConfig: 'bla',
200-
environment: 'unit-test-env',
201-
repoName: undefined,
202-
orgName: 'SomeAwesomeCoder',
191+
environment: ENVIRONMENT,
192+
runnerType: 'Org',
193+
runnerOwner: ORG_NAME,
203194
});
204195
expect(mockSSM.putParameter).not.toBeCalled();
205196
});

Diff for: ‎modules/runners/lambdas/runners/src/scale-runners/runners.ts

+9-12
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ export interface RunnerInfo {
88
}
99

1010
export interface ListRunnerFilters {
11-
repoName?: string;
12-
orgName?: string;
13-
environment?: string;
11+
runnerType?: 'Org' | 'Repo';
12+
runnerOwner?: string;
13+
environment: string | undefined;
1414
}
1515

1616
export async function listRunners(filters: ListRunnerFilters | undefined = undefined): Promise<RunnerInfo[]> {
@@ -23,11 +23,8 @@ export async function listRunners(filters: ListRunnerFilters | undefined = undef
2323
if (filters.environment !== undefined) {
2424
ec2Filters.push({ Name: 'tag:Environment', Values: [filters.environment] });
2525
}
26-
if (filters.repoName !== undefined) {
27-
ec2Filters.push({ Name: 'tag:Repo', Values: [filters.repoName] });
28-
}
29-
if (filters.orgName !== undefined) {
30-
ec2Filters.push({ Name: 'tag:Org', Values: [filters.orgName] });
26+
if (filters.runnerType && filters.runnerOwner) {
27+
ec2Filters.push({ Name: `tag:${filters.runnerType}`, Values: [filters.runnerOwner] });
3128
}
3229
}
3330
const runningInstances = await ec2.describeInstances({ Filters: ec2Filters }).promise();
@@ -52,8 +49,8 @@ export async function listRunners(filters: ListRunnerFilters | undefined = undef
5249
export interface RunnerInputParameters {
5350
runnerConfig: string;
5451
environment: string;
55-
repoName?: string;
56-
orgName?: string;
52+
runnerType: 'Org' | 'Repo';
53+
runnerOwner: string;
5754
}
5855

5956
export async function terminateRunner(runner: RunnerInfo): Promise<void> {
@@ -89,8 +86,8 @@ export async function createRunner(runnerParameters: RunnerInputParameters): Pro
8986
Tags: [
9087
{ Key: 'Application', Value: 'github-action-runner' },
9188
{
92-
Key: runnerParameters.orgName ? 'Org' : 'Repo',
93-
Value: runnerParameters.orgName ? runnerParameters.orgName : runnerParameters.repoName,
89+
Key: runnerParameters.runnerType,
90+
Value: runnerParameters.runnerOwner
9491
},
9592
],
9693
},

Diff for: ‎modules/runners/lambdas/runners/src/scale-runners/scale-down.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ export async function scaleDown(): Promise<void> {
139139
// list and sort runners, newest first. This ensure we keep the newest runners longer.
140140
const runners = (
141141
await listRunners({
142-
environment: environment,
142+
environment
143143
})
144144
).sort((a, b): number => {
145145
if (a.launchTime === undefined) return 1;

Diff for: ‎modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts

+24-20
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,8 @@ describe('scaleUp with GHES', () => {
126126
await scaleUp('aws:sqs', TEST_DATA);
127127
expect(listRunners).toBeCalledWith({
128128
environment: 'unit-test-environment',
129-
repoName: undefined,
129+
runnerType: 'Org',
130+
runnerOwner: TEST_DATA.repositoryOwner
130131
});
131132
});
132133

@@ -174,8 +175,8 @@ describe('scaleUp with GHES', () => {
174175
expect(createRunner).toBeCalledWith({
175176
environment: 'unit-test-environment',
176177
runnerConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} --token 1234abcd `,
177-
orgName: TEST_DATA.repositoryOwner,
178-
repoName: undefined,
178+
runnerType: 'Org',
179+
runnerOwner: TEST_DATA.repositoryOwner,
179180
});
180181
});
181182

@@ -187,8 +188,8 @@ describe('scaleUp with GHES', () => {
187188
environment: 'unit-test-environment',
188189
runnerConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} ` +
189190
`--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`,
190-
orgName: TEST_DATA.repositoryOwner,
191-
repoName: undefined,
191+
runnerType: 'Org',
192+
runnerOwner: TEST_DATA.repositoryOwner,
192193
});
193194
});
194195
});
@@ -202,7 +203,8 @@ describe('scaleUp with GHES', () => {
202203
await scaleUp('aws:sqs', TEST_DATA);
203204
expect(listRunners).toBeCalledWith({
204205
environment: 'unit-test-environment',
205-
repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
206+
runnerType: 'Repo',
207+
runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
206208
});
207209
});
208210

@@ -253,8 +255,8 @@ describe('scaleUp with GHES', () => {
253255
runnerConfig: `--url ` +
254256
`https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` +
255257
`--token 1234abcd --labels label1,label2`,
256-
orgName: undefined,
257-
repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
258+
runnerType: 'Repo',
259+
runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
258260
});
259261
});
260262

@@ -267,8 +269,8 @@ describe('scaleUp with GHES', () => {
267269
runnerConfig: `--url ` +
268270
`https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` +
269271
`--token 1234abcd --labels label1,label2`,
270-
orgName: undefined,
271-
repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
272+
runnerType: 'Repo',
273+
runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
272274
});
273275
});
274276
});
@@ -322,7 +324,8 @@ describe('scaleUp with public GH', () => {
322324
await scaleUp('aws:sqs', TEST_DATA);
323325
expect(listRunners).toBeCalledWith({
324326
environment: 'unit-test-environment',
325-
repoName: undefined,
327+
runnerType: 'Org',
328+
runnerOwner: TEST_DATA.repositoryOwner
326329
});
327330
});
328331

@@ -345,8 +348,8 @@ describe('scaleUp with public GH', () => {
345348
expect(createRunner).toBeCalledWith({
346349
environment: 'unit-test-environment',
347350
runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} --token 1234abcd `,
348-
orgName: TEST_DATA.repositoryOwner,
349-
repoName: undefined,
351+
runnerType: 'Org',
352+
runnerOwner: TEST_DATA.repositoryOwner
350353
});
351354
});
352355

@@ -358,8 +361,8 @@ describe('scaleUp with public GH', () => {
358361
environment: 'unit-test-environment',
359362
runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} ` +
360363
`--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`,
361-
orgName: TEST_DATA.repositoryOwner,
362-
repoName: undefined,
364+
runnerType: 'Org',
365+
runnerOwner: TEST_DATA.repositoryOwner
363366
});
364367
});
365368
});
@@ -373,7 +376,8 @@ describe('scaleUp with public GH', () => {
373376
await scaleUp('aws:sqs', TEST_DATA);
374377
expect(listRunners).toBeCalledWith({
375378
environment: 'unit-test-environment',
376-
repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
379+
runnerType: 'Repo',
380+
runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
377381
});
378382
});
379383

@@ -414,8 +418,8 @@ describe('scaleUp with public GH', () => {
414418
environment: 'unit-test-environment',
415419
runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` +
416420
`--token 1234abcd --labels label1,label2`,
417-
orgName: undefined,
418-
repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
421+
runnerType: 'Repo',
422+
runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
419423
});
420424
});
421425

@@ -427,8 +431,8 @@ describe('scaleUp with public GH', () => {
427431
environment: 'unit-test-environment',
428432
runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` +
429433
`--token 1234abcd --labels label1,label2`,
430-
orgName: undefined,
431-
repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
434+
runnerType: 'Repo',
435+
runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`,
432436
});
433437
});
434438
});

0 commit comments

Comments
 (0)