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

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

Conversation

OffensiveBias-08-145
Copy link
Contributor

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:

service.resources.Resources[warmerConfig.role] = {
Type: 'AWS::IAM::Role',
Properties: {
Path: '/',
RoleName: {
'Fn::Join': [
'-',
[
service.service,
stage,
{ Ref: 'AWS::Region' },
warmerName.toLowerCase(),
'role',
],
],
},

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
Copy link
Contributor Author

@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.

Copy link
Owner

@juanjoDiaz juanjoDiaz 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 the very complete PR.

It's much appreciate when a PR describes well the reasoning behind it and it follows the style, includes tests and a good commit description 🙂

Just a couple of minor changes in the docs requested

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@juanjoDiaz
Copy link
Owner

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
Copy link
Contributor Author

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.

Path: '/',
RoleName: warmerConfig.roleName ? warmerConfig.roleName : {
'Fn::Join': [
'-',
warmerConfig.roleName ? [warmerConfig.roleName]
: [
service.service,
stage,
{ Ref: 'AWS::Region' },
warmerName.toLowerCase(),
'role',
],
],

Any thoughts?

@icholy
Copy link
Contributor

icholy commented Sep 27, 2022

Nice, I just ran into this issue too.

src/warmer.js Outdated Show resolved Hide resolved
@juanjoDiaz
Copy link
Owner

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.

@juanjoDiaz juanjoDiaz merged commit 6dc2dba into juanjoDiaz:master Oct 4, 2022
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