fix(events): archive construct does not have defaultChild set#21345
fix(events): archive construct does not have defaultChild set#21345mergify[bot] merged 6 commits intoaws:mainfrom
Conversation
|
|
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
This feels(?) like a breaking change but I can't actually tell if it is. Part of the problem is that no integration tests were written for this module. Even though this is a fix, not a feat, I think we need an integration test here.
|
@TheRealAmazonKendra Yeah maybe you are right.. Do you think about some logical ID matching stuff out in the wild? Should I introduce a feature flag for this then? It would feel weird to make a feature flag for a fix tbh.. What would be the best way to keep backwards compatibility? I will definitely add Integration tests! And btw: is this even the best way to define the |
Pull request has been modified.
|
Sorry @rix0rrr - I have now changed the code and implemented your suggestion. |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Just one quick comment on the unit test and I'd still like to see an integ tests added.
Pull request has been modified.
|
@TheRealAmazonKendra Thanks for your feedback :) I have added an integration test and changed the unit test according to your review. |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Thank you for adding the tests. We just need the integration test to use the new IntegTest construct, per the integration testing guide.
Pull request has been modified.
|
@TheRealAmazonKendra I have added the |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Looks great!
|
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). |
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). |
Fixes #21263
Adds id 'Default' to CfnArchive in Archive construct.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license