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

Option to disable checking of API Gateway CloudWatch role setting #7333

Merged
merged 3 commits into from
Feb 18, 2020
Merged

Option to disable checking of API Gateway CloudWatch role setting #7333

merged 3 commits into from
Feb 18, 2020

Conversation

coyoteecd
Copy link
Contributor

@coyoteecd coyoteecd commented Feb 13, 2020

What did you implement

Provides a roleManagedExternally setting to enable/disable checking of API Gateway CloudWatch role when using provider.logs to enable API Gateway logs. This avoids creating a custom resource lambda (which requires a number of extra permissions assigned to your cfnRole just to run it). You must ensure the setting is there through other means, otherwise the deployment will fail when trying to enable the logs.

Also documents the logging choices available under the Logs section in the API Gateway events doc page.

See also discussion here.

Closes #6973

How can we verify it

provider:
  logs:
    restApi:
       roleManagedExternally: true

Deploying a serverless.yml with roleManagedExternally: false should not create a custom resource. Deployment should succeed when API Gateway already has a CloudWatch role ARN configured in that region (and fail otherwise, obviously).

Default is false, to avoid making it a breaking change.

Todos

Useful Scripts
  • npm run test:ci --> Run all validation checks on proposed changes
  • npm run lint:updated --> Lint all the updated files
  • npm run lint:fix --> Automatically fix lint problems (if possible)
  • npm run prettier-check:updated --> Check if updated files adhere to Prettier config
  • npm run prettify:updated --> Prettify all the updated files
  • Write and run all tests
  • Write documentation
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

Copy link
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.

Great thanks @coyoteecd, definitely a worthwhile improvement. Please see my remarks

docs/providers/aws/guide/functions.md Outdated Show resolved Hide resolved
docs/providers/aws/guide/serverless.yml.md Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

Merging #7333 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7333      +/-   ##
==========================================
+ Coverage   87.92%   87.97%   +0.04%     
==========================================
  Files         240      240              
  Lines        8897     8929      +32     
==========================================
+ Hits         7823     7855      +32     
  Misses       1074     1074
Impacted Files Coverage Δ
...mpile/events/lib/ensureApiGatewayCloudWatchRole.js 100% <100%> (ø) ⬆️
...lugins/aws/package/compile/events/httpApi/index.js 88.78% <0%> (+4.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3910df1...050c9ff. Read the comment docs.

@coyoteecd
Copy link
Contributor Author

Made updates, ready for review

Copy link
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.

Thanks @coyoteecd, looks really good. Just proposed few style improvements, and we should be good to go

docs/providers/aws/events/apigateway.md Outdated Show resolved Hide resolved
docs/providers/aws/events/apigateway.md Outdated Show resolved Hide resolved
Copy link
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.

Thank you @coyoteecd !

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.

Enabling API Gateway Logs in serverless.yml tries to create new IAM Role and a new Lambda
3 participants