feat(s3): constructs factories - create well architected s3 buckets #1110
Conversation
biffgaut
requested review from
hayesry,
mobri2a,
emcfins,
mickychetta and
georgebearden
as code owners
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| export interface S3BucketFactoryProps { | ||
| readonly bucketProps?: s3.BucketProps | any, | ||
| readonly logS3AccessLogs?: boolean, | ||
| readonly loggingBucketProps?: s3.BucketProps, | ||
| } | ||
|
|
||
| // Create a response specifically for the interface to avoid coupling client with internal implementation | ||
| export interface S3BucketFactoryResponse { | ||
| readonly s3Bucket: s3.Bucket, | ||
| readonly s3LoggingBucket?: s3.Bucket, | ||
| } |
There was a problem hiding this comment.
This is just a house keeping thing, but over time this list of inputs/outputs to the factories will get very large. Would it make more sense to move them to their own file, one file for each set? Just something to contemplate, not even sure if its the right answer.
There was a problem hiding this comment.
I thought about that - this approach will become absolutely unmanageable as we expose more factories and must change. But I postponed devising an approach to handle multiple factories until I had more examples (rather than trying to come up with the generic from a single example).
| if (props) { | ||
| defaults.CheckS3Props(props); | ||
| } |
There was a problem hiding this comment.
It would be nice if defaults.CheckS3Props could handle undefined, to avoid having to put these calls in the factory methods.
| bucketProps: props.bucketProps, | ||
| loggingBucketProps: props.loggingBucketProps, | ||
| logS3AccessLogs: props.logS3AccessLogs, |
There was a problem hiding this comment.
as these fields are exactly the same, would we want to use the spread operator instead to simplify the logic?
const propsArg: defaults.BuildS3BucketProps = props ? { ...props } : {}
There was a problem hiding this comment.
Yes, they are the same - but is that an fundamental trait or a coincidence? The factory is supposed to put an abstraction over the core function - feels like using the spread operator because of the coincidence will increase coupling between the exposed factory interface and the core function.
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/awslabs/aws-solutions-constructs.git", | ||
| "directory": "source/patterns/@aws-solutions-constructs/aws-dynamodbstreams-lambda" |
There was a problem hiding this comment.
this directory looks wrong "aws-dynamodbstreams-lambda"
| @@ -173,6 +173,7 @@ export interface BuildS3BucketResponse { | |||
|
|
|||
| /** | |||
| * @internal This is an internal core function and should not be called directly by Solutions Constructs clients. | |||
| * @internal This functionality is exposed externally through aws-constructs-factories | |||
There was a problem hiding this comment.
nice comment for redirection
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -18,6 +18,8 @@ | |||
|
|
|||
| The core library includes the basic building blocks of the AWS Solutions Constructs Library. It defines the core classes that are used in the rest of the AWS Solutions Constructs Library. | |||
|
|
|||
| > NOTE: Functions in the core library are not part of the published interface for Solutions Constructs. While they are not hidden, using them directly can result in breaking changes outside the scope of a Major release. As many users have expressed an interest in accessing this functionality, we are in the process of exposing this functionality through factories that will produce individual well architected resources. Find the current state of this effort under `aws-constructs-factories`. | |||
There was a problem hiding this comment.
super nit: can remove one of the two spaces between "Major" and "release" in "Major release"
There was a problem hiding this comment.
Looks good, no concerns from my end. Test coverage is good, covers different use patterns with sensible assertions. Verified no required props on the S3Bucket that we should be aware of that might get in the way of using the construct at this level. If something presents, we can address it as part of an issue but all core functionality seems to be easily override-able by the user if needed. Able to discuss any aspects further, but I'm happy with this so far. Nice work 👍
(Let me know if I should approve or if you're just seeking comment-based feedback at this point in time)
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Description of changes:
This PR introduces a brand new functionality in Solutions Constructs - Constructs Factories. All Solutions Constructs are built upon our underlying core library which creates well architected resources from a single service (which our L3 Solutions Constructs then connect). Constructs Factories expose our core library functionality in a documented, supported way and is the preferred way to access the functionality in our core library.
This first release creates a factory for S3 buckets, allowing creation of a bucket following all best practices with a single call. We plan to add SQS queues and Step Functions state machines in the near future.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.