Skip to content

Conversation

@MarcioMeier
Copy link
Collaborator

Summary

Proposal usage for #17

@MarcioMeier MarcioMeier requested a review from flaviostutz May 7, 2024 15:38
@MarcioMeier MarcioMeier self-assigned this May 7, 2024
Copy link
Owner

@flaviostutz flaviostutz left a comment

Choose a reason for hiding this comment

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

For me the component interface is nice. What do you think @erik-am @sergioflores-j?

@@ -0,0 +1,78 @@
## Construct StaticS3Website

This construct creates an S3 bucket and upload any content into it.
Copy link
Owner

Choose a reason for hiding this comment

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

Todo: change to "This construct creates a S3 bucket and uploads the contents of a local folder to it"

- [AWS S3 Bucket lib](https://github.com/aws/aws-cdk/tree/main/packages/aws-cdk-lib/aws-s3)
- [AWS S3 Bucket deployment lib](https://github.com/aws/aws-cdk/tree/main/packages/aws-cdk-lib/aws-s3-deployment)

Why to use it over the CDK constructs?
Copy link
Owner

Choose a reason for hiding this comment

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

discussion: I would remove this paragraph as there isn't another construct with a similar behavior, so this is not "better", it's simply a new composition.

deployments: [
{
sources: [Source.asset('./dist', { exclude: ['*.html'] })],
cacheControl: [CacheControl.fromString('public,max-age=31536000,immutable')],
Copy link
Owner

Choose a reason for hiding this comment

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

Question: this will make the GET of the files on S3 to have response header "cache-control" set? If so, pls add this comment to the example

Copy link
Collaborator Author

@MarcioMeier MarcioMeier May 8, 2024

Choose a reason for hiding this comment

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

It will add the cache control metadata to the objects that will be uploaded by the sources selection.
In short, yes, it will add the cache-control headers to the files


// Attaches the policy to the deployment bucket
staticWebsite.bucket.addToResourcePolicy(ipLimitPolicy)
```
Copy link
Owner

Choose a reason for hiding this comment

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

Praise: The "complex" example is very nice!

```ts
// This will create the bucket `my-website-bucket` and upload the files in your local `dist` folder into it
new StaticS3Website(stack, 'my-website', {
bucketName: 'my-website-bucket',
Copy link
Owner

Choose a reason for hiding this comment

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

Do the bucket name defaults to the id of the construct or it's a required prop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather have it as a required prop, so it be more explicit when building/reading the construct.
But having it as default is indeed an option.

@MarcioMeier MarcioMeier marked this pull request as draft May 8, 2024 07:38
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