Skip to content
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

Behaviour Prefixes (for CDK v2 upgrade) #1401

Merged
merged 6 commits into from
Nov 4, 2024
Merged

Conversation

finnholland
Copy link
Collaborator

@finnholland finnholland commented Oct 14, 2024

Description of the proposed changes

  • Added an id tag to prerender function names to avoid duplicate named function issues
  • Export remap path from construct for easier uniformity with types
  • behaviourPrefixes to allow for behaviours on regional prefixes such as au/ or nz/

Notes to reviewers

  • These changes allow for the CDK v2 upgrade to work for multiple brands, impact on other clients untested but it is all conditional so shouldn't affect them

🛈 When you've finished leaving feedback, please add a final comment to the PR tagging the author, letting them know that you have finished leaving feedback

@finnholland finnholland changed the title Behaviour Prefixes (for RAG v2 upgrade) Behaviour Prefixes (for CDK v2 upgrade) Oct 14, 2024
additionalBehaviors[`${prefix.prefix}*`] = {
origin: s3Origin,
viewerProtocolPolicy: ViewerProtocolPolicy.REDIRECT_TO_HTTPS,
edgeLambdas: prefix.edgeLambdas,
edgeLambdas: prefix.behaviourOverride.edgeLambdas,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of just passing the edge lambda you can do this:

      additionalBehaviors[`${prefix.prefix}*`] = {
        origin: s3Origin,
        viewerProtocolPolicy: ViewerProtocolPolicy.REDIRECT_TO_HTTPS,
        originRequestPolicy: originRequestPolicy,
        cachePolicy: originCachePolicy,
        responseHeadersPolicy: responseHeadersPolicy,
        ...prefix.behaviourOverride,
      };

Because that will then allow you to override any of the properties if you need

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@finn-holland-aligent this one hasn't been resolved yet

@@ -140,11 +140,14 @@ export interface StaticHostingProps {
enableStaticFileRemap?: boolean;

/**
* Any prefixes to remapping that should be included in the path such as au or nz
* Overrides default behaviour paths with a prefix and takes in options to apply to each static file behaviour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options don't apply to the static file behaviour do they?

Copy link
Contributor

@TheOrangePuff TheOrangePuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@finnholland finnholland merged commit 255177e into main Nov 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants