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

Fix Event Streams when functions have direct reference to custom roles #3457

Merged
merged 7 commits into from
Apr 13, 2017

Conversation

piohhmy
Copy link
Contributor

@piohhmy piohhmy commented Apr 12, 2017

What did you implement:

Closes #3142

The AWS::Lambda::EventSourceMapping resource was being created with wrong the DependsOn attribute if custom roles were defined using a string to reference a logical role name elsewhere defined in the serverless.yml. The EventSourceMapping resource was previously only checking for roles defined via a direct ARN or the Fn::GetAtt syntax. It did not support referencing a role's name directly (i.e.. role: myDefaultRole as documented here)

The result of this is that the EventSourceMapping resource would set the DependsOn to the default IamRoleLambdaExecution role. If custom roles were used for all functions then the IamRoleLambdaExecution role would not exist in the compiled cloud formation and the deploy would fail with Template format error: Unresolved resource dependencies [IamPolicyLambdaExecution] in the Resources block of the template

How did you implement it:

This builds upon #3083 as it enables the custom role to be referenced directly without using the Fn::GetAtt syntax.

How can we verify it:

Deploy any serverless.yml with an event stream using only functions with custom roles (e.g. the sample in #3142 )

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

@piohhmy piohhmy changed the title Fix depends on with event stream Fix Event Streams when functions have direct reference to custom roles Apr 12, 2017
@piohhmy
Copy link
Contributor Author

piohhmy commented Apr 12, 2017

Note: There may be a good refactoring opportunity here to keep things DRY. The code at https://github.com/serverless/serverless/blob/master/lib/plugins/aws/deploy/compile/functions/index.js#L32 is suspiciously similar to the if statement touched in this PR.

@pmuens pmuens self-requested a review April 12, 2017 11:38
Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Thanks for submitting a fix @piohhmy 👍

I was able to reproduce #3142 locally and this PR fixes the issue 🎉

Merging...

@pmuens pmuens merged commit aebce86 into serverless:master Apr 13, 2017
@pmuens pmuens modified the milestone: 1.12 Apr 13, 2017
@piohhmy piohhmy deleted the fix-depends-on-with-event-stream branch April 13, 2017 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants