Skip to content

Commit acbaebc

Browse files
committed
fix: widen handler type to TerminationWatcherEvent union
Address review feedback: - Add InstanceStateChangeEvent and TerminationWatcherEvent union to types.d.ts - Update handle() and lambda handler to accept the union type so the detail-type check is no longer a compile-time constant - Replace unsafe cast in test with a properly-shaped InstanceStateChangeEvent fixture including state-change-specific detail fields
1 parent 5c65915 commit acbaebc

4 files changed

Lines changed: 43 additions & 26 deletions

File tree

lambdas/functions/termination-watcher/src/lambda.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,12 @@ import { Context, SQSEvent } from 'aws-lambda';
66
import { handle as handleTerminationWarning } from './termination-warning';
77
import { handle as handleTermination } from './termination';
88
import { handleDeregisterRetry, DeregisterRetryMessage } from './deregister';
9-
import { BidEvictedDetail, BidEvictedEvent, SpotInterruptionWarning, SpotTerminationDetail } from './types';
9+
import { BidEvictedDetail, BidEvictedEvent, TerminationWatcherEvent } from './types';
1010
import { Config } from './ConfigResolver';
1111

1212
const config = new Config();
1313

14-
export async function interruptionWarning(
15-
event: SpotInterruptionWarning<SpotTerminationDetail>,
16-
context: Context,
17-
): Promise<void> {
14+
export async function interruptionWarning(event: TerminationWatcherEvent, context: Context): Promise<void> {
1815
setContext(context, 'lambda.ts');
1916
logger.logEventIfEnabled(event);
2017
logger.debug('Configuration of the lambda', { config });

lambdas/functions/termination-watcher/src/termination-warning.test.ts

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { EC2Client, Instance } from '@aws-sdk/client-ec2';
22
import { mockClient } from 'aws-sdk-client-mock';
33
import 'aws-sdk-client-mock-jest';
44
import { handle } from './termination-warning';
5-
import { SpotInterruptionWarning, SpotTerminationDetail } from './types';
5+
import { SpotInterruptionWarning, SpotTerminationDetail, InstanceStateChangeEvent, InstanceStateChangeDetail } from './types';
66
import { metricEvent } from './metric-event';
77
import { deregisterRunner } from './deregister';
88

@@ -36,7 +36,7 @@ const config = {
3636
ghesApiUrl: '',
3737
};
3838

39-
const event: SpotInterruptionWarning<SpotTerminationDetail> = {
39+
const spotEvent: SpotInterruptionWarning<SpotTerminationDetail> = {
4040
version: '0',
4141
id: '1',
4242
'detail-type': 'EC2 Spot Instance Interruption Warning',
@@ -51,8 +51,23 @@ const event: SpotInterruptionWarning<SpotTerminationDetail> = {
5151
},
5252
};
5353

54+
const stateChangeEvent: InstanceStateChangeEvent = {
55+
version: '0',
56+
id: '2',
57+
'detail-type': 'EC2 Instance State-change Notification',
58+
source: 'aws.ec2',
59+
account: '123456789012',
60+
time: '2015-11-11T21:30:00Z',
61+
region: 'us-east-1',
62+
resources: ['arn:aws:ec2:us-east-1b:instance/i-abcd1111'],
63+
detail: {
64+
'instance-id': 'i-abcd1111',
65+
state: 'shutting-down',
66+
},
67+
};
68+
5469
const instance: Instance = {
55-
InstanceId: event.detail['instance-id'],
70+
InstanceId: 'i-abcd1111',
5671
InstanceType: 't2.micro',
5772
Tags: [
5873
{ Key: 'Name', Value: 'test-instance' },
@@ -68,28 +83,27 @@ describe('handle termination warning', () => {
6883
vi.clearAllMocks();
6984
});
7085

71-
it('should log and create a metric for spot interruption events', async () => {
86+
it('should emit metric for spot interruption events', async () => {
7287
vi.mocked(getInstances).mockResolvedValue([instance]);
73-
await handle(event, config);
88+
await handle(spotEvent, config);
7489

75-
expect(metricEvent).toHaveBeenCalled();
76-
expect(metricEvent).toHaveBeenCalledWith(instance, event, 'SpotInterruptionWarning', expect.anything());
90+
expect(metricEvent).toHaveBeenCalledWith(instance, spotEvent, 'SpotInterruptionWarning', expect.anything());
7791
expect(deregisterRunner).toHaveBeenCalledWith(instance, config);
7892
});
7993

80-
it('should log details and not create a metric', async () => {
94+
it('should not emit metric when createSpotWarningMetric is false', async () => {
8195
vi.mocked(getInstances).mockResolvedValue([instance]);
8296

8397
const noMetricConfig = { ...config, createSpotWarningMetric: false };
84-
await handle(event, noMetricConfig);
85-
expect(metricEvent).toHaveBeenCalledWith(instance, event, undefined, expect.anything());
98+
await handle(spotEvent, noMetricConfig);
99+
expect(metricEvent).toHaveBeenCalledWith(instance, spotEvent, undefined, expect.anything());
86100
expect(deregisterRunner).toHaveBeenCalledWith(instance, noMetricConfig);
87101
});
88102

89-
it('should not create a metric if filter not matched.', async () => {
103+
it('should not emit metric or deregister if filter not matched', async () => {
90104
vi.mocked(getInstances).mockResolvedValue([instance]);
91105

92-
await handle(event, {
106+
await handle(spotEvent, {
93107
createSpotWarningMetric: true,
94108
createSpotTerminationMetric: false,
95109
tagFilters: { 'ghr:environment': '_NO_MATCH_' },
@@ -105,12 +119,7 @@ describe('handle termination warning', () => {
105119
it('should not emit metric for instance state-change events but still deregister', async () => {
106120
vi.mocked(getInstances).mockResolvedValue([instance]);
107121

108-
const stateChangeEvent = {
109-
...event,
110-
'detail-type': 'EC2 Instance State-change Notification' as const,
111-
};
112-
113-
await handle(stateChangeEvent as unknown as SpotInterruptionWarning<SpotTerminationDetail>, config);
122+
await handle(stateChangeEvent, config);
114123

115124
expect(metricEvent).toHaveBeenCalledWith(instance, stateChangeEvent, undefined, expect.anything());
116125
expect(deregisterRunner).toHaveBeenCalledWith(instance, config);

lambdas/functions/termination-watcher/src/termination-warning.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { createChildLogger, getTracedAWSV3Client } from '@aws-github-runner/aws-powertools-util';
2-
import { SpotInterruptionWarning, SpotTerminationDetail } from './types';
2+
import { TerminationWatcherEvent } from './types';
33
import { EC2Client, Instance } from '@aws-sdk/client-ec2';
44
import { Config } from './ConfigResolver';
55
import { tagFilter, getInstances } from './ec2';
@@ -8,7 +8,7 @@ import { deregisterRunner } from './deregister';
88

99
const logger = createChildLogger('termination-warning');
1010

11-
async function handle(event: SpotInterruptionWarning<SpotTerminationDetail>, config: Config): Promise<void> {
11+
async function handle(event: TerminationWatcherEvent, config: Config): Promise<void> {
1212
logger.debug('Received spot notification warning:', { event });
1313
const ec2 = getTracedAWSV3Client(new EC2Client({ region: process.env.AWS_REGION }));
1414
const instances = await getInstances(ec2, [event.detail['instance-id']]);
@@ -19,7 +19,7 @@ async function handle(event: SpotInterruptionWarning<SpotTerminationDetail>, con
1919

2020
async function createMetricForInstances(
2121
instances: Instance[],
22-
event: SpotInterruptionWarning<SpotTerminationDetail>,
22+
event: TerminationWatcherEvent,
2323
config: Config,
2424
): Promise<void> {
2525
for (const instance of instances) {

lambdas/functions/termination-watcher/src/types.d.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,14 @@ interface UserIdentity {
4242
interface ServiceEventDetails {
4343
instanceIdSet: string[];
4444
}
45+
46+
export interface InstanceStateChangeDetail {
47+
'instance-id': string;
48+
state: string;
49+
}
50+
51+
// eslint-disable-next-line @typescript-eslint/no-empty-object-type
52+
export interface InstanceStateChangeEvent
53+
extends EventBridgeEvent<'EC2 Instance State-change Notification', InstanceStateChangeDetail> {}
54+
55+
export type TerminationWatcherEvent = SpotInterruptionWarning<SpotTerminationDetail> | InstanceStateChangeEvent;

0 commit comments

Comments
 (0)