-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix(cloudfront): add region to autogenerated name for cloudfront origin request policy #34348
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?
fix(cloudfront): add region to autogenerated name for cloudfront origin request policy #34348
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.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
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.
✅ A exemption request has been requested. Please wait for a maintainer's review.
Exemption Request:
The existing integration test for cache policy & origin request policy explicitly set the resource name properties Line 33 in 7330f91
Not sure if you'd want to see the autogenerated name behavior in the integration suite or not? |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -94,7 +94,7 @@ export class OriginRequestPolicy extends Resource implements IOriginRequestPolic | |||
// Enhanced CDK Analytics Telemetry | |||
addConstructMetadata(this, props); | |||
|
|||
const originRequestPolicyName = props.originRequestPolicyName ?? Names.uniqueId(this); | |||
const originRequestPolicyName = props.originRequestPolicyName ?? `${Names.uniqueId(this).slice(0, 110)}-${Stack.of(this).region}`; |
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.
can you confirm whether this will cause replacement of the existing resource ?
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.
Docs state that changing the OriginRequestPolicyConfig require no interruption.
Furthermore, changing the Name inside the OriginRequestPolicyConfig also says it requires no interruption.
Thanks for identifying the missing integ test gap, i think we should add new one for this use-case. |
Issue # (if applicable)
Closes #34336
Reason for this change
So that a stack defining an origin request policy can be easily deployed into more than one region in a single account.
Description of changes
Followed the same naming convention already in place for cache policies, which was added in #13737
Describe any new or updated permissions being added
Description of how you validated changes
Adjusted an existing unit test to recognize the new name
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license