-
Notifications
You must be signed in to change notification settings - Fork 4k
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: migrate @aws-cdk-testing/cli-integ
from sdk v2 to sdk v3
#30046
Conversation
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
"@aws-sdk/client-cloudformation": "3.421.0", | ||
"@aws-sdk/client-s3": "3.421.0", | ||
"@aws-sdk/client-ecr": "3.421.0", | ||
"@aws-sdk/client-ecs": "3.451.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really weird. If we mention 3.421.0
it leads to test errors in packages/@aws-cdk/custom-resource-handlers/test/aws-events-targets/aws-api-handler/aws-api-handler.test.ts
. Unfortunately, we had set the client version to 3.451.0
in there.
I see there is a lot of mismatch between sdk versions in different package.json
s and we need to align them to same version. We cannot lower them to a previous version, so probably will end up aligning to the version that is the highest in our package.
I see some integ tests not running since they are unable to find credentials:
This is due to a different path for config defined in |
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
Signed-off-by: Vinayak Kukreja <[email protected]>
@aws-cdk-testing/cli-integ
from sdk v2 to sdk v3
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
await this.aws.deleteStacks(...stacksToDelete.map(s => s.StackName)); | ||
await this.aws.deleteStacks(...stacksToDelete.map(s => { | ||
if (!s.StackName) { | ||
throw new Error('Stack name is required to delete a stack.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be throwing an error here? I don't see that we did this before... I'm wondering if throwing here would cause resources to leak in the case that this error is thrown?
if (!role.Role) { | ||
throw new Error('Role not found'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as my above comment. Why add the behavior of throwing error?
@Mergifyio update |
✅ Branch has been successfully updated |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Comments on closed issues and PRs are hard for our team to see. |
Closes #29837
Reason for this change
We are migrating our code from SDK v2 to SDK v3. This change is part of that effort.
Description of changes
We used to have an abstracted caller since SDK v2 did not have separated clients. Since SDK v3 now have separated clients, I am also making code changes to reflect that and not moving forwards with the abstracted client vendor.
Also, updated the imports and references to work with SDK v3 and introduced
ini
for readingaws config
files, since SDK v3 changed the functionality from sync to async and to add that to our code would mean a lot more of changes for adding a simple functionalityDescription of how you validated changes
The run through test pipeline is successful.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license