fix(cloudfront): propagate `originAccessControlId` CloudFront Origin property to CloudFormation templates by ivanbarlog · Pull Request #32020 · aws/aws-cdk

ivanbarlog · 2024-11-05T06:11:56Z

Issue

Closes #32018.

Reason for this change

The originAccessControlId property of CloudFront Origin has not been propagated to CloudFormation templates.

Description of changes

Propagate the property to render function.

Description of how you validated changes

have run the run build over the whole repo - my computer almost exploded.

I have also run tests for the aws-cloudfront which run successfully.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

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.

ivanbarlog · 2024-11-05T10:27:04Z

Exemption Request

I've just fixed the place where the value which supposed to be rendered in the CloudFormation template was missing. It was set in the L2 construct but not propagated to the CloudFormation template.

I don't really see how can I write test for this.

Can you please guide me with this?

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

gracelu0

Thanks for opening a PR! I left a comment on updating the tests. I don't think we need an integration test here so once unit tests are updated I can add the exemption label.

gracelu0 · 2024-11-19T23:38:12Z

...ws-cdk-testing/framework-integ/test/aws-cloudfront-origins/test/integ.function-url-origin.ts

-  defaultBehavior: { origin: new origins.FunctionUrlOrigin(fnUrl) },
+  defaultBehavior: {
+    origin: new origins.FunctionUrlOrigin(fnUrl, {
+      originAccessControlId: oac.attrId,


I think we can add a new unit test instead of updating this integration test. For the unit test I'd suggest:

adding a new function to test-origin.ts here which returns a test origin with originAccessControlId defined

adding a unit test to distribution.test.ts which sets up a distribution using the test origin from above and check that the Origin property in DistributionConfig contains OriginAccessControlId (can reference existing unit test here)

Let me know if you need more guidance on this.

Hi @gracelu0, thanks for the feedback!

I have removed the integration tests (which your automated CI forced me to write) and I hope I've added the unit tests correctly. It has run for me so let's see what CI will tell us :-)

Thanks for help!

Pull request has been modified.

aws-cdk-automation

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.

gracelu0 · 2024-11-21T22:43:46Z

@ivanbarlog I think the build failed due to some lint errors, try running yarn lint --fix from the root to fix the formatting issues and then I believe the build should pass

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

codecov · 2024-11-22T22:14:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.19%. Comparing base (9f3d09b) to head (9b58405).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32020   +/-   ##
=======================================
  Coverage   78.19%   78.19%           
=======================================
  Files         105      105           
  Lines        7176     7176           
  Branches     1318     1318           
=======================================
  Hits         5611     5611           
  Misses       1377     1377           
  Partials      188      188

Flag	Coverage Δ
suite.unit	`78.19% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`78.19% <ø> (ø)`

…Formation templates fixes aws#32018

gracelu0

Thank you for contributing!

gracelu0 · 2024-11-27T22:24:51Z

@Mergifyio update

mergify · 2024-11-27T22:25:01Z

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/codecov.yml without workflows permission

mergify · 2024-11-27T22:25:02Z

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).

Pull request has been modified.

aws-cdk-automation · 2024-11-28T00:21:24Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 9b58405
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-11-28T00:21:48Z

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).

github-actions · 2024-11-28T00:22:09Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. labels Nov 5, 2024

aws-cdk-automation requested a review from a team November 5, 2024 06:12

github-actions bot added the p2 label Nov 5, 2024

aws-cdk-automation previously requested changes Nov 5, 2024

View reviewed changes

ivanbarlog changed the title ~~propagate originAccessControlId CloudFront Origin property to CloudFormation templates~~ fix(cloudfront origin): propagate originAccessControlId CloudFront Origin property to CloudFormation templates Nov 5, 2024

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Nov 5, 2024

ivanbarlog force-pushed the issue-32018 branch from 198bbb8 to e357ad3 Compare November 6, 2024 08:03

ivanbarlog mentioned this pull request Nov 7, 2024

aws-cloudfront: setting originAccessControlId in aws-cloudfront-origins does nothing #32018

Closed

1 task

ivanbarlog force-pushed the issue-32018 branch from e357ad3 to edaa470 Compare November 7, 2024 17:54

ivanbarlog changed the title ~~fix(cloudfront origin): propagate originAccessControlId CloudFront Origin property to CloudFormation templates~~ fix(cloudfront): propagate originAccessControlId CloudFront Origin property to CloudFormation templates Nov 7, 2024

github-actions bot added the effort/small Small work item – less than a day of effort label Nov 7, 2024

ivanbarlog force-pushed the issue-32018 branch 3 times, most recently from 7878d5a to 46c5e96 Compare November 13, 2024 16:29

ivanbarlog force-pushed the issue-32018 branch from 46c5e96 to 2bc6e3f Compare November 17, 2024 21:33

gracelu0 previously requested changes Nov 19, 2024

View reviewed changes

ivanbarlog force-pushed the issue-32018 branch from 2bc6e3f to 8989e83 Compare November 21, 2024 08:49

ivanbarlog force-pushed the issue-32018 branch from 8989e83 to 91da02b Compare November 21, 2024 08:51

aws-cdk-automation previously requested changes Nov 21, 2024

View reviewed changes

gracelu0 added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 21, 2024

ivanbarlog force-pushed the issue-32018 branch from e6d0d12 to 2ab6331 Compare November 22, 2024 15:39

gracelu0 removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Nov 22, 2024

Ivan Barlog added 3 commits November 24, 2024 10:40

propagate originAccessControlId CloudFront Origin property to Cloud…

f371db4

…Formation templates fixes aws#32018

add unit tests

708934f

lint files

0603410

ivanbarlog force-pushed the issue-32018 branch from 2ab6331 to 0603410 Compare November 24, 2024 09:40

gracelu0 previously approved these changes Nov 27, 2024

View reviewed changes

Merge branch 'main' into issue-32018

9b58405

gracelu0 approved these changes Nov 28, 2024

View reviewed changes

mergify bot merged commit f9708a6 into aws:main Nov 28, 2024

github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2024

Conversation

ivanbarlog commented Nov 5, 2024

Issue

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

ivanbarlog commented Nov 5, 2024

Uh oh!

gracelu0 left a comment

Choose a reason for hiding this comment

Uh oh!

gracelu0 Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

ivanbarlog Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

gracelu0 commented Nov 21, 2024

Uh oh!

codecov bot commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gracelu0 left a comment

Choose a reason for hiding this comment

Uh oh!

gracelu0 commented Nov 27, 2024

Uh oh!

mergify bot commented Nov 27, 2024

❌ Mergify doesn't have permission to update

Uh oh!

mergify bot commented Nov 27, 2024

Uh oh!

aws-cdk-automation commented Nov 28, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Nov 28, 2024

Uh oh!

github-actions bot commented Nov 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 22, 2024 •

edited

Loading