Skip to content

Add support for existing Cognito User Pools#6362

Merged
pmuens merged 1 commit intomasterfrom
existing-cognito-user-pools
Jul 18, 2019
Merged

Add support for existing Cognito User Pools#6362
pmuens merged 1 commit intomasterfrom
existing-cognito-user-pools

Conversation

@pmuens
Copy link
Copy Markdown
Contributor

@pmuens pmuens commented Jul 11, 2019

This PR adds support for existing Cognito User Pools via a custom resource.

What did you implement:

Closes #4207
Closes #5401
Refs #4241
Refs #5642
Refs #6290

/cc @exoego

How did you implement it:

Checks the existing config property of the cognitoUserPool event and swaps out the Cognito User Pool CloudFormation resource with a custom resource the Serverless Framework provides. The custom resource basically does raw SDK calls to update the existing Cognito User Pool. 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:
      - cognitoUserPool:
          pool: ExistingUserPool
          trigger: CustomMessage
          existing: true

Todos:

Note: Run npm run test-ci to run all validation checks on proposed changes

  • Write tests and confirm existing functionality is not broken.
    Validate via npm test
  • Write documentation
  • Ensure there are no lint errors.
    Validate via npm run lint-updated
    Note: Some reported issues can be automatically fixed by running npm run lint:fix
  • Ensure introduced changes match Prettier formatting.
    Validate via npm run prettier-check-updated
    Note: All reported issues can be automatically fixed by running npm run prettify-updated
  • 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.48.0 milestone Jul 11, 2019
@pmuens pmuens self-assigned this Jul 11, 2019
@pmuens pmuens force-pushed the existing-cognito-user-pools branch 2 times, most recently from 7db8b73 to 8d6f1de Compare July 12, 2019 11:13
@pmuens pmuens force-pushed the existing-cognito-user-pools branch 5 times, most recently from 315679f to 49ccc72 Compare July 16, 2019 12:58
@pmuens pmuens requested a review from medikoo July 16, 2019 12:58
@pmuens pmuens marked this pull request as ready for review July 16, 2019 12:59
@pmuens pmuens force-pushed the existing-cognito-user-pools branch from 49ccc72 to 71ef724 Compare July 17, 2019 12:47
@pmuens
Copy link
Copy Markdown
Contributor Author

pmuens commented Jul 17, 2019

Thanks for the reviews @medikoo and @exoego 👍

I just addressed your issues and updated the PR accordingly. Other than that I also added support for the usage of multiple custom resources in one service.

@pmuens pmuens force-pushed the existing-cognito-user-pools branch from 71ef724 to b929c82 Compare July 17, 2019 13:23
@pmuens pmuens force-pushed the existing-cognito-user-pools branch from b929c82 to 63c070c Compare July 18, 2019 10:55
@pmuens pmuens requested a review from medikoo July 18, 2019 11:05
Copy link
Copy Markdown
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

LGTM, still on stylistic note, it'll be good to reduce lodash usage.

I see in many places _.merge where Object.assign works, or _.forEach(array), where we can directly do array.forEach or for (const item of array)

I think it'll be good to favor native functions over external utils (where they do not bring added value)

@pmuens
Copy link
Copy Markdown
Contributor Author

pmuens commented Jul 18, 2019

Thanks for the review @medikoo 👍

I agree that we should definitely try to adhere to vanilla JS as much as possible given that we dropped support for all the old Node.js versions.

@pmuens pmuens merged commit 9a84d18 into master Jul 18, 2019
@pmuens pmuens deleted the existing-cognito-user-pools branch July 18, 2019 11:22
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.

The pool: <POOL_ID> property should also support Ref Cannot reference existing Cognito User Pool resource in configuration

3 participants