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

Conversation

b177y
Copy link

@b177y b177y commented Aug 2, 2024

Issue #29838

Progresses (not closes) issue #29838

Reason for this change

CDK uses the function getEndpointSuffix in the AWS JavaScript V2 SDK which is not a private function but is not a publicly documented and supported function of the SDK. The SDK maintainers have pointed out that this should not be used. The V2 SDK is entering maintenance mode on 2024-09-08, with end of support on 2025-09-08.

CDK maintains the region-info package which can provide the domainSuffix property of a region: https://docs.aws.amazon.com/cdk/api/v2/docs/@aws-cdk_region-info.RegionInfo.html

Description of changes

This change replaces the call to the unsupported aws-sdk/lib/region_config.getEndpointSuffix with @aws-cdk/region-info.RegionInfo.get(region).domainSuffix.

Description of how you validated changes

No new unit or integration tests have been added as this does not add or change any functionality. A random integration test has been run to verify this works. When the integration test is run, CDK uses the updated function to work out the S3 URL for the CloudFormation template file. This deploys successfully.

yarn integ --parallel-regions eu-west-2 --force --profiles dev --directory /Users/USERNAME/code/custom-aws-cdk/packages/@aws-cdk-testing/framework-integ/test aws-iam/test/integ.role.js
yarn run v1.22.21
$ integ-runner --language javascript --parallel-regions eu-west-2 --force --profiles dev --directory /Users/USERNAME/code/custom-aws-cdk/packages/@aws-cdk-testing/framework-integ/test aws-iam/test/integ.role.js

Verifying integration test snapshots...

  UNCHANGED  aws-iam/test/integ.role 0.424s

Snapshot Results:

Tests:    1 passed, 1 total

Running integration tests for failed tests...

Running in parallel across profiles dev and regions: eu-west-2
Running test /Users/USERNAME/code/custom-aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.role.js in dev/eu-west-2
  SUCCESS    aws-iam/test/integ.role-integ-iam-role/DefaultTest 83.549s
       NO ASSERTIONS

Test Results:

Tests:    1 passed, 1 total
✨  Done in 84.95s.

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Aug 2, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team August 2, 2024 10:48
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Aug 2, 2024
@b177y b177y changed the title chore(aws-cdk): use RegionInfo instead of aws-sdk v2 for region domain suffix chore(cdk): use RegionInfo instead of aws-sdk v2 for region domain suffix Aug 2, 2024
Copy link

@kuhe kuhe left a comment

Choose a reason for hiding this comment

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

This PR lgtm from the perspective of the AWS SDK JS team.

@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@b177y
Copy link
Author

b177y commented Aug 7, 2024

CodeQL Analyser is failing with:

Severe disk cache trouble (corruption or out of space) at /home/runner/work/_temp/codeql_databases/javascript/db-javascript/default/cache/pages/1b/9c.pack: Failed to write item to disk

Which is the same issue as in PR #30991 - I am assuming this error is related to the configuration of the test runner rather than anything to do with the code changes in these PRs.

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 6a8d658
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/31008/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@b177y
Copy link
Author

b177y commented Aug 20, 2024

Closing PR as this is being implemented as part of a larger change to CDK.

@b177y b177y closed this Aug 20, 2024
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr/needs-cli-test-run This PR needs CLI tests run against it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants