Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(cdk): use RegionInfo instead of aws-sdk v2 for region domain suffix #31008

Closed
wants to merge 8 commits into from
22 changes: 6 additions & 16 deletions packages/aws-cdk/lib/api/aws-auth/sdk.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as region_info from '@aws-cdk/region-info';
import * as AWS from 'aws-sdk';
import type { ConfigurationOptions } from 'aws-sdk/lib/config-base';
import { debug, trace } from './_env';
Expand All @@ -6,21 +7,6 @@ import { cached } from './cached';
import { Account } from './sdk-provider';
import { traceMethods } from '../../util/tracing';

// We need to map regions to domain suffixes, and the SDK already has a function to do this.
// It's not part of the public API, but it's also unlikely to go away.
//
// Reuse that function, and add a safety check, so we don't accidentally break if they ever
// refactor that away.

/* eslint-disable @typescript-eslint/no-require-imports */
const regionUtil = require('aws-sdk/lib/region_config');
require('aws-sdk/lib/maintenance_mode_message').suppress = true;
/* eslint-enable @typescript-eslint/no-require-imports */

if (!regionUtil.getEndpointSuffix) {
Copy link
Contributor

@comcalvi comcalvi Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our unit tests mock this function so it can be tested. Please remove those mocks and ensure the tests still pass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I removed this but it caused test/api/logs/logs-monitor.test.ts to fail.

I have it passing by changing the expected num of stderr errors to 2, and pointing the check of the error content to the second index. Let me know if there's a different approach you would rather I take with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you removed the mock and that caused that test to fail? What did it fail with?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When removing the line require('aws-sdk/lib/maintenance_mode_message').suppress = true;, the test suite fails with a message that it was expecting one stderr message. I have left the test in that was failing, and have got it to pass by setting it to expect 2 stderr messages

This commit passes the tests without any tests / mocks being removed:
07d4e30

throw new Error('This version of AWS SDK for JS does not have the \'getEndpointSuffix\' function!');
}

export interface ISDK {
/**
* The region this SDK has been instantiated for
Expand Down Expand Up @@ -291,7 +277,11 @@ export class SDK implements ISDK {
}

public getEndpointSuffix(region: string): string {
return regionUtil.getEndpointSuffix(region);
const suffix = region_info.RegionInfo.get(region).domainSuffix;
if (!suffix) {
throw new Error(`Could not find domainSuffix in RegionInfo for ${region}`);
}
return suffix;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk/test/api/logs/logs-monitor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ test('process events', async () => {

// THEN
const expectedLocaleTimeString = eventDate.toLocaleTimeString();
expect(stderrMock).toHaveBeenCalledTimes(1);
expect(stderrMock.mock.calls[0][0]).toContain(
expect(stderrMock).toHaveBeenCalledTimes(2);
expect(stderrMock.mock.calls[1][0]).toContain(
`[${blue('loggroup')}] ${yellow(expectedLocaleTimeString)} message`,
);
});
Expand Down
Loading