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

server-side-website with additional S3 Origin behaviour #325

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

aran112000
Copy link
Contributor

@aran112000 aran112000 commented Apr 24, 2023

This adds support to create additional CloudFront Origins & Behaviours so you can map additional paths to alternative, existing S3 buckets.

Includes updated documentation to show how this can be used.

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

Awesome, that looks like a great idea that will benefit many! (the "/uploads/" example is very common)

I added some suggestions inline. Could you also add some tests? This is a large feature so we want to make sure it is covered.

src/constructs/aws/ServerSideWebsite.ts Outdated Show resolved Hide resolved
src/constructs/aws/ServerSideWebsite.ts Outdated Show resolved Hide resolved
@aran112000 aran112000 requested a review from mnapoli April 27, 2023 07:49
@aran112000 aran112000 requested a review from mnapoli June 8, 2023 16:20
@mnapoli
Copy link
Member

mnapoli commented Jun 16, 2023

Hi, just bumping here regarding my last comment: I think the only thing missing here for a merge are tests? Especially since you discovered plenty of edge cases, maintaining this feature sounds like it will be quite some effort, so the tests will help minimize the effort 😅

@aran112000
Copy link
Contributor Author

Hi, just bumping here regarding my last comment: I think the only thing missing here for a merge are tests? Especially since you discovered plenty of edge cases, maintaining this feature sounds like it will be quite some effort, so the tests will help minimize the effort 😅

Sure thing, @robchett just added these now :)

@evo-chris
Copy link

@mnapoli Just wondering if there is a plan in mind for the next release and will it include this PR? If there's anything we can help with, let us know. Thanks.

@aran112000
Copy link
Contributor Author

Hey @fredericbarthelet, @mnapoli

Any chance this could be reviewed / merged if you're happy with the change & tests added here please?

@robchett robchett force-pushed the server-side-website-merge-custom-s3-origins branch from 14993f6 to fba77bd Compare December 12, 2023 13:24
@kevincerro
Copy link
Contributor

kevincerro commented Dec 17, 2023

Hope that this will be finally merged.
I need to host under a subpath /blog a static-website using a server-side-website construct

@vikivyas
Copy link

Aran Reeks (@aran112000) while It is not being approved, how can we use this?

will uploaded files to the bucket be removed on the next deployment?

@aran112000
Copy link
Contributor Author

Aran Reeks (@aran112000) while It is not being approved, how can we use this?

will uploaded files to the bucket be removed on the next deployment?

Feel free to use my fork of Lift, we use this on production on a number of sites successfully. It doesn't clear any assets from other buckets on deployment. They're just added as origins to your Cloudfront distributions.

@mnapoli - is it possible to complete the review and get this merged, please?

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.

6 participants