Conditionally adds cloudformation template

etgrieco commented 6 years ago

Potential solution to issue/request https://github.com/leftclickben/serverless-api-stage/issues/13

leftclickben commented 6 years ago

Thanks for the contributions! So sorry I haven't had a chance to review this yet. I will take a proper look later tonight.

leftclickben commented 6 years ago

I agree with #13 and this is a good solution that involves minimal code change.

However I'm conflicted about a few things:

The behaviour when you don't specify the MethodSettings, whether it should use the "defaults" or not, like you pointed out
The fact that DataTraceEnabled is true in the defaults for MethodSettings (line 130 on this branch), which itself would imply that the logging permissions are required by default (and probably this shouldn't be the case)
You have also uncovered another problem with the permissions: it's missing cloudwatch:PutMetricData, which is required when MetricsEnabled is true (and if the permissions are applied conditionally as per this PR, the logs:* permissions should be applied only on LoggingLevel and DataTraceEnabled)
At it stands, I think this PR is already BC breaking (at least concerning README.md, although the existing tests haven't been changed)... I'm not opposed to that, rather I'm thinking we can go further with it, since it's a major version bump already

@etgrieco thanks again for this. Let me think about the above, and I will get back to you. I'm leaning towards extending these changes further and making it 2.x, but it will need a migration guide and support for both major versions, at least for a while.

leftclickben / serverless-api-stage

Conditionally adds cloudformation template #14