-
Notifications
You must be signed in to change notification settings - Fork 0
POST /api/reports #54
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
Conversation
adamrefaey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GuidoBR Nice work! We just need to make a few changes
backend/src/iac/backend-stack.ts
Outdated
| conditions: { | ||
| // Restrict uploads to PDF and JPG files | ||
| StringLike: { | ||
| 's3:x-amz-content-type': ['application/pdf', 'image/jpeg', 'image/jpg'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to have validation here since that means we have the validation in many places and when we want to make any kind of change like add more file types, we'll have to change multiple places.
backend/src/iac/backend-stack.ts
Outdated
| const uploadBucket = new s3.Bucket(this, `${appName}UploadBucket-${props.environment}`, { | ||
| bucketName: `${appName.toLowerCase()}-uploads-${props.environment}-${this.account}`, | ||
| removalPolicy: RemovalPolicy.RETAIN, | ||
| versioned: true, // Enable versioning in production |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we don't need to use versioning.
adamrefaey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @GuidoBR !
Change
Does this PR introduce a breaking change?
{...}
What needs to be documented once your changes are merged?
{...}
Additional Comments
{...}