search

juanjoDiaz / serverless-plugin-warmup

Keep your lambdas warm during winter. ♨
MIT License
1.11k stars 115 forks source link

feat: optionally set warmer roleName if using default role #321

Closed OffensiveBias-08-145 closed 2 years ago

OffensiveBias-08-145 commented 2 years ago

Optionally set a roleName for the warmers.

Use Case:

Sometimes the default generated Role name exceeds the IAM max name length of 64 characters.

This causes deployments to fail for some users if they do not disable the warmers for deployments.

Example:

Error:
CREATE_FAILED: WarmUpPluginDefaultRole (AWS::IAM::Role)
1 validation error detected: Value 'apple-map-api-sandbox-ap-south-1-stack-sandbox-ap-south-1-default-role' at 'roleName' failed to satisfy constraint: Member must have length less than or equal to 64 (Service: AmazonIdentityManagement; Status Code: 400; Error Code: ValidationError; Request ID: 60b62c0f-18d8-4b63-80fe-672b1b5d8add; Proxy: null)

Cause:

https://github.com/juanjoDiaz/serverless-plugin-warmup/blob/847b45cc675e8f12ea439b68283417feab562ef9/src/warmer.js#L25-L40

Fix:

Optionally provide warmer RoleName field for two main reasons:

  1. Allow users to follow a specific naming structure across their accounts
  2. Provide a workaround for the max character limit for IAM Role names.

Please let me know if I have missed anything... Thanks!

OffensiveBias-08-145 commented 2 years ago

@juanjoDiaz Please let me know if I left out any contribution steps or if there are any test cases you feel need to be added.

juanjoDiaz commented 2 years ago

It seems that coverage is going down.

Can you check npm run test-with-coverage and investigate what is not being covered?

OffensiveBias-08-145 commented 2 years ago

It seems that coverage is going down.

Can you check npm run test-with-coverage and investigate what is not being covered?

Seems as though L:32 of warmer.js is uncovered. I tried adding a test for coverage if the roleName was undefined but it seems to have not resolved the issue.

https://github.com/juanjoDiaz/serverless-plugin-warmup/blob/c9c343775f656a1921697c5e7b7752c52fac6055/src/warmer.js#L28-L40

Any thoughts?

icholy commented 2 years ago

Nice, I just ran into this issue too.

juanjoDiaz commented 2 years ago

Sorry for the delay. This is almost there.

Just a small issue captured by test coverage. Once you fix it this is good to go.