flaviostutz / cdk-practical-constructs

A collection of CDK constructs for making the development of AWS based applications easier and safer in a practical way
MIT License
2 stars 2 forks source link

Feature Request: Deployment S3 bucket #17

Open MarcioMeier opened 2 months ago

MarcioMeier commented 2 months ago

Problem being solved

Deploying any kind of source code into AWS S3, such as react apps.

Proposal

We create an S3 bucket for the deployment and upload the code from given path to it.

I thought on something like this codesnippet:

import { Construct } from 'constructs';
import { RemovalPolicy } from 'aws-cdk-lib';
import { Bucket, BlockPublicAccess, BucketProps }  from 'aws-cdk-lib/aws-s3';
import { BucketDeployment, Source, BucketDeploymentProps }  from 'aws-cdk-lib/aws-s3-deployment';

export class S3Bucket extends Construct {
  public readonly bucket: Bucket;

  constructor(scope: Construct, id: string, props: BucketProps) {
    super(scope, id, props);

    this.bucket = new Bucket(this, id, {
      versioned: false,
      bucketName: props.bucketName,
      publicReadAccess: false,
      blockPublicAccess: BlockPublicAccess.BLOCK_ALL,
      enforceSSL: true,
      minimumTLSVersion: 1.2,
      removalPolicy: RemovalPolicy.DESTROY,
      autoDeleteObjects: true,
      ...props,
    });
  }
}

type S3BucketDeploymentProps = {
  bucketConfig: BucketProps;
  deploymentConfig: Omit<BucketDeploymentProps, 'destinationBucket'>;
};

export class S3BucketDeployment extends Construct {
  constructor(scope: Construct, id: string, props: S3BucketDeploymentProps) {
    super(scope, id, props);

    const bucket = new S3Bucket(this, ${id}-bucket, props.bucketConfig);

    new BucketDeployment(this, ${id}-bucket-deployment, {
      sources: [Source.asset('./dist')],
      destinationBucket: bucket,
      ...props.deploymentConfig,
    });
  }
}

It still needs refinement over the default props/polciies, etc..

Further reading:

Out of scope

flaviostutz commented 2 months ago

This is a very useful construct!

I wouldn't call it "S3BucketDeployment" because "deployment" is more related to a "process", not to a "resource", which is what we want to define here (also with its lifecycle managed by CloudFormation).

What do you think of the name "StaticS3Website"?

Also I didn't get why exporting S3Bucket, as this seems an internal class used just to add defaults to the bucket.

We could externalise (this.bucket etc) the instance of the Bucket and the BucketDeployment for further changes if the user needs so.

My suggestion is to create a first PR with only the readme file showing a usage of the lib so we can discuss/refine from the user perspective how we want it to be, what do you think?

@erik-am @sergioflores-j pls take a look at this if possible too!

erik-am commented 2 months ago

I already created something similar, so it's definitely useful to have something like this. I will share my version through other channels.

I like the "destroy" retain policy. Makes it easy to clean-up Pull Request deployments, and there's nothing to be lost, because one can always re-deploy static assets! 😃

totolicious commented 2 months ago

Would it be useful to remove sources: [Source.asset('./dist')] ? Feels like this should be configurable. Or maybe at least provide it as a default, which would mean maybe to change the type like this?

type S3BucketDeploymentProps = {
  bucketConfig: BucketProps;
  deploymentConfig: Omit<BucketDeploymentProps, 'destinationBucket' | 'sources'>;
  sources?: BucketDeploymentProps['sources']
};

Also, I had to change destinationBucket: bucket to destinationBucket: bucket.bucket because of type errors

flaviostutz commented 2 months ago

You mean "source" should be required, right? I think it shouldn't have a default value as it's very dependent on the build strategy of the user and if he/she forgets to set it, he/she will see "strange" errors like "./dist folder not found" when he/she doesn't have any dist folder in his mind.

totolicious commented 2 months ago

Yeah, i guess removing source from here would be better. Agree with the strange './dist' error due to the default value.

MarcioMeier commented 2 months ago

I agree, sources should not have default value.