-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat: add support for aws-logs.LogGroup.deletionProtectionEnabled #36583
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
base: main
Are you sure you want to change the base?
Conversation
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 review is outdated)
|
I'm trying to run the integration test according to INTEGRATION_TESTS.md to also create the snapshot, but unfortunately don't get it done successfully.
Appreciate any help. |
|
@robert-hanuschke Are you specifying the test file accurately using a relative path from the current directory? |
|
I am in the test directory of the module, the .ts file is there, for the integ call I just replace the ending with .js as I understood the instructions
|
|
@robert-hanuschke you have to build the integ framework first, execute this command at the root of the repo after this is done you should be able to run |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
@dalatrex2 thank you! This was connected to the errors I reported in the initial PR text. First, the lerna command failed simiarly. I now found out that I was on node 25 in my terminal. After setting it to the lts version, the commands succeed. My command outputs look fine - the rosetta script is complaining about the unrelated module aws-amplify-alpha yarn test aws-logs
yarn integ test/aws-logs/test/integ.loggroup-deletionprotection.js --update-on-failed
/bin/bash ./scripts/run-rosetta.sh
yarn test (shortened)
|
Issue #36554
Closes #36554
This is my very first PR for aws-cdk. Any hints on what I could do better are greatly appreciated.
I am not aware whether I would have to do something additionally to also have DeletionProtectionEnabled be available on the L1 resource CfnLogGroup, but am happy to do so if instructed.
Reason for this change
aws-logs.LogGroup did not yet support the DeletionProtectionEnabled property. This PR adds it.
Description of changes
This is a standard change to support a boolean property that was added in CloudFormation.
I added the property to the interface LogGroupProps, copying the description from the CloudFormation definition. I also extended the initialization of CfnLogGroup in the constructor. Finally, I created a unit test for the new property.
Describe any new or updated permissions being added
No further changes required.
Description of how you validated changes
Added a unit test which ran successfully.
The following are unrelated to my change, but I wanted to have them mentioned in case it points to me setting up something wrongly in my local environment and introducing errors down the line.
Following the commands in CONTRIBUTING.MD:
The command
npx lerna run build --skip-nx-cachehad 16/17 targets succeed, with the one failing being:Likewise,
yarn buildin the packages/aws-cdk-lib folder failed with messages likebut
yarn testcompleted successfullyChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license