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

Add support for existing S3 buckets #6290

Merged
merged 1 commit into from Jul 9, 2019

Conversation

Projects
5 participants
@pmuens
Copy link
Member

commented Jun 21, 2019

This PR adds support for existing S3 buckets via a custom resource.

What did you implement:

Refs #4241

How did you implement it:

Checks the existing config property of the s3 event and swaps out the S3 CloudFormation resource with a custom resource the Serverless Framework provides. The custom resource basically does raw SDK calls to update the existing S3 bucket. This way everything is encapsulated via CloudFormation and the stack won't get out of sync if we implement it via raw SDK calls.

How can we verify it:

service: test

provider:
  name: aws
  runtime: nodejs10.x

functions:
  test:
    handler: handler.hello
    events:
      - s3:
          bucket: existing-s3-bucket
          event: s3:ObjectCreated:*
          rules:
            - prefix: uploads
            - suffix: .jpg
          existing: true

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens added this to the 1.46.0 milestone Jun 21, 2019

@pmuens pmuens self-assigned this Jun 21, 2019

@pmuens pmuens added this to In progress in Serverless via automation Jun 21, 2019

@pmuens pmuens force-pushed the existing-s3-buckets branch from a33a1f4 to ff0b827 Jun 21, 2019

@raeesbhatti

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Looking forward to this. Thanks 👍🏼

@garethmcc

This comment has been minimized.

Copy link

commented Jun 21, 2019

I know many many community members who are looking forward to this, myself included. Nice one!!!!

@pmuens pmuens force-pushed the existing-s3-buckets branch 4 times, most recently from 15cb9a7 to ac4ae56 Jun 24, 2019

@medikoo medikoo modified the milestones: 1.46.0, 1.47.0 Jun 26, 2019

@pmuens pmuens force-pushed the existing-s3-buckets branch from ac4ae56 to b12b918 Jun 26, 2019

@pmuens pmuens force-pushed the existing-s3-buckets branch 12 times, most recently from 1af5360 to af66bdd Jun 26, 2019

@pmuens pmuens force-pushed the existing-s3-buckets branch 5 times, most recently from b7d78d4 to d05d7a1 Jul 2, 2019

@pmuens
Copy link
Member Author

left a comment

@medikoo thanks for the review. I've addressed most of your comments. In haven't addressed all of your review comments but will do so once I work on the support for existing Cognito user pools.

While working on the PR I also added an integration test which tests various S3 setups.

Let me know if there's anything else I should address.

@pmuens pmuens requested a review from medikoo Jul 8, 2019

Show resolved Hide resolved lib/plugins/aws/customResources/resources/s3/handler.js Outdated
Show resolved Hide resolved lib/plugins/aws/package/compile/events/s3/index.js Outdated
Show resolved Hide resolved lib/plugins/aws/package/compile/events/s3/index.js Outdated
Show resolved Hide resolved tests/integration-all/s3/service/core.js Outdated
Show resolved Hide resolved tests/integration-all/s3/tests.js Outdated

@pmuens pmuens force-pushed the existing-s3-buckets branch 2 times, most recently from 49f7870 to 737679c Jul 9, 2019

@pmuens pmuens requested a review from medikoo Jul 9, 2019

@pmuens
Copy link
Member Author

left a comment

@medikoo thanks for the review. I just updated the PR according to your review.

We're now awaiting function logs (checking for markers) rather than using a delayed Promise. Furthermore the handler is now wrapped so that response calls are not repeated in every single function.

@medikoo
Copy link
Member

left a comment

@pmuens Looks great!

Just few minor clarifications and should be LGTM

Show resolved Hide resolved tests/integration-all/s3/service/utils.js
Show resolved Hide resolved tests/integration-all/s3/service/utils.js Outdated

@pmuens pmuens force-pushed the existing-s3-buckets branch from 737679c to d09b754 Jul 9, 2019

@pmuens pmuens requested a review from medikoo Jul 9, 2019

Serverless automation moved this from In progress to Reviewer approved Jul 9, 2019

@medikoo

medikoo approved these changes Jul 9, 2019

Copy link
Member

left a comment

LGTM!

@pmuens pmuens merged commit 6156af4 into master Jul 9, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (medikoo) No manifest changes detected

Serverless automation moved this from Reviewer approved to Done Jul 9, 2019

@pmuens pmuens deleted the existing-s3-buckets branch Jul 9, 2019

@shawn271828

This comment has been minimized.

Copy link

commented Jul 13, 2019

Great job!

I came across searching for this feature and have discovered a tiny issue:

Resources in policy statement have hard-coded xxx:aws:xxx.
This seems to be an issue in some region like China when it is xxx:aws-cn:xxx

@pmuens

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

Thanks for the feedback @shawn271828 👍

Can you open up an issue for this so that we can track and address it? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.