-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(s3): resolve synthesis error in BucketPolicy.fromCfnBucketPolicy() #35633
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
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)
| @@ -1,3 +1,13 @@ | |||
| import test from 'node:test'; | |||
| import test from 'node:test'; | |||
| import test from 'node:test'; | |||
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.
| import test from 'node:test'; |
| import test from 'node:test'; | ||
| import test from 'node:test'; | ||
| import { describe } from 'node:test'; | ||
| import test from 'node:test'; |
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.
| import test from 'node:test'; |
| import test from 'node:test'; | ||
| import { describe } from 'node:test'; | ||
| import test from 'node:test'; | ||
| import test from 'node:test'; |
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.
| import test from 'node:test'; |
| import { describe } from 'node:test'; | ||
| import test from 'node:test'; | ||
| import test from 'node:test'; | ||
| import test from 'node:test'; |
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.
| import test from 'node:test'; |
- Fix synthesis error where duplicate CfnBucketPolicy was created without policyDocument - Ensure PolicyDocument is properly passed from original CfnBucketPolicy to constructor - Add test to verify synthesis works and duplicate resources are created as expected - Maintains existing behavior while fixing synthesis bug Fixes synthesis error: 'Supplied properties not correct for CfnBucketPolicyProps: policyDocument: required but missing'
b3ab538 to
1aafcbc
Compare
Remove duplicate 'import test from node:test' statements that were accidentally added during rebase/merge
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
kaizencc
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.
looks good! just a minor comment about the test itself. actual code looks great :)
- Remove redundant app.synth() call since Template.fromStack() already synthesizes - Add more comprehensive validation of policy documents - Improve test clarity and comments - Address reviewer feedback from kaizencc
|
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). |
|
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. |
|
This issue has been reopened and is now available for discussion. |
|
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). |
|
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). |
|
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. |
Issue # (if applicable)
Closes #34322
Reason for this change
Fixes synthesis error: 'Supplied properties not correct for CfnBucketPolicyProps: policyDocument: required but missing'
Description of changes
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license