-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat: set log retention of all CDK-vended custom resources with CustomResourceConfig #31041
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
Conversation
aws-cdk-automation
left a comment
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.
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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
comcalvi
left a comment
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 a great start! I have a few comments though 🙂
Please add an integration test (in @aws-cdk-testing), move this code to aws-cdk-lib/custom-resources and update that module's README to tell users how to use the new feature.
packages/@aws-cdk/custom-resource-handlers/lib/custom-resources-framework/classes.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/custom-resource-handlers/lib/custom-resources-framework/classes.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/custom-resource-handlers/lib/custom-resources-framework/classes.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/custom-resource-handlers/lib/custom-resources-framework/classes.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/custom-resource-handlers/lib/custom-resources-framework/classes.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/custom-resource-config/custom-resource-config.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/custom-resource-config/test/custom-resource-config.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/custom-resource-config/test/custom-resource-config.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/custom-resource-config/test/custom-resource-config.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/custom-resource-config/test/custom-resource-config.test.ts
Outdated
Show resolved
Hide resolved
|
Also please update the title to be a |
aws-cdk-automation
left a comment
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.
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.
packages/aws-cdk-lib/custom-resources/lib/custom-resource-config/custom-resource-config.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/custom-resources/lib/custom-resource-config/custom-resource-config.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/custom-resources/lib/custom-resource-config/custom-resource-config.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/custom-resources/lib/custom-resource-config/custom-resource-config.ts
Outdated
Show resolved
Hide resolved
...ages/aws-cdk-lib/custom-resources/test/custom-resource-config/custom-resource-config.test.ts
Show resolved
Hide resolved
...ages/aws-cdk-lib/custom-resources/test/custom-resource-config/custom-resource-config.test.ts
Outdated
Show resolved
Hide resolved
...ages/aws-cdk-lib/custom-resources/test/custom-resource-config/custom-resource-config.test.ts
Outdated
Show resolved
Hide resolved
...ages/aws-cdk-lib/custom-resources/test/custom-resource-config/custom-resource-config.test.ts
Outdated
Show resolved
Hide resolved
...ages/aws-cdk-lib/custom-resources/test/custom-resource-config/custom-resource-config.test.ts
Show resolved
Hide resolved
|
This is very close to merging 🙂 we're almost there! |
comcalvi
left a comment
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.
Nice work! 🎉
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Reason for this change
CDK vended custom resources may or may not have Cloudwatch logGroup set with a retention period.
Modify all CDK-vended custom resources with
CustomResourceConfigand to modify retention period withaddLogRetentionLifetimemethod.CustomResourceConfig.of(app).addLogRetentionLifetime(logs.RetentionDays.TEN_YEARS);Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license