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

Merge LogGroup Resources, even when no Role resources were merged #3213

Merged
merged 1 commit into from
Feb 9, 2017

Conversation

eahefnawy
Copy link
Member

What did you implement:

Closes #3174

Fixed a bug where LogGroup resources are not created if all your functions use custom roles.

How did you implement it:

Moved the logic that adds LogGroups resources at the top of the merge method, so that they're always merged, even when no roles are merged, like in the case of custom roles.

How can we verify it:

Using this config:

service: log-group-bug

provider:
  name: aws

functions:
  hello:
    role: something
    handler: handler.hello

  world:
    role: somethingelse
    handler: handler.world

All functions use custom roles, so no role resources are added. Run sls deploy --noDeploy and you'll see that the roles resources are not added, while the LogGroup resources are added.

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

@eahefnawy eahefnawy added this to the 1.7 milestone Feb 8, 2017
@eahefnawy eahefnawy self-assigned this Feb 8, 2017
@pmuens pmuens self-requested a review February 8, 2017 15:01
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.

Tested it with the following config:

service: service

provider:
  name: aws
  runtime: nodejs4.3

functions:
  hello:
    handler: handler.hello
    role: foo

The LogGroup resource is created, but also the Role for the function (which I think should be fine):

"HelloLambdaFunction": {
      "Type": "AWS::Lambda::Function",
      "Properties": {
        "Code": {
          "S3Bucket": {
            "Ref": "ServerlessDeploymentBucket"
          },
          "S3Key": "serverless/service/dev/1486634119276-2017-02-09T09:55:19.276Z/service.zip"
        },
        "FunctionName": "service-dev-hello",
        "Handler": "handler.hello",
        "MemorySize": 1024,
        "Role": {
          "Fn::GetAtt": [
            "foo",
            "Arn"
          ]
        },
        "Runtime": "nodejs4.3",
        "Timeout": 6
      },
      "DependsOn": [
        "foo"
      ]
    },

Maybe I'm missing smth. but this shouldn't be the case according to this comment in the PR description:

All functions use custom roles, so no role resources are added. Run sls deploy --noDeploy and you'll see that the roles resources are not added, while the LogGroup resources are added.

According to the tests you mean that this role is not added (which I can confirm is not added):

getRoleLogicalId() {
    return 'IamRoleLambdaExecution';
  },

@eahefnawy
Copy link
Member Author

eahefnawy commented Feb 9, 2017

@pmuens yep sorry for the confusion there. I mean the default role resource is not added. But of course the custom role is added, otherwise the function wouldn't have any permissions at all. So the output you're seeing is what's expected.

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.

@eahefnawy Ok, got it. That's what I would expect as well... Merging this now since it resolves the issue!

@pmuens pmuens merged commit 5effce8 into master Feb 9, 2017
@pmuens pmuens deleted the fix-log-group-bug branch February 9, 2017 12:40
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.

None yet

3 participants