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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 8 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5a4ad4e
chore(aws-cdk): use RegionInfo instead of aws-sdk v2 for region domain
Aug 2, 2024
1869fdf
chore(aws-cdk): use RegionInfo instead of aws-sdk v2 for region domai…
Aug 2, 2024
bf4b1ce
Merge branch 'dev-region-domain-suffix' of github.com:b177y/aws-cdk i…
Aug 2, 2024
bca982f
Merge branch 'main' into dev-region-domain-suffix
b177y Aug 7, 2024
90ebdd6
Merge branch 'main' into dev-region-domain-suffix
b177y Aug 12, 2024
07d4e30
fix: remove suppress deprecation warning message
Aug 13, 2024
3244db7
Merge branch 'main' into dev-region-domain-suffix
b177y Aug 13, 2024
6a8d658
Merge branch 'main' into dev-region-domain-suffix
b177y Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 6 additions & 16 deletions packages/aws-cdk/lib/api/aws-auth/sdk.ts
View file
Open in desktop
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 •
edited
Loading

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
Loading