agiledigital / serverless-sns-sqs-lambda

serverless plugin to make serverless-sns-sqs-lambda events
Apache License 2.0
81 stars 18 forks source link

Function names with dashes aren't supported #814

Open lyuboraykov opened 8 months ago

lyuboraykov commented 8 months ago

When using it on a function with dashes in its name, I'm getting a Cloudformation error on deploy:

Error:
The CloudFormation template is invalid: Template format error: Resource name Calculate-session-metricsEventSourceMappingSQSCalculateSessionMetricsQueue is non alphanumeric.

The function configuration is:

calculate-session-metrics:
  timeout: 900
  maximumEventAge: 1800
  handler:  ...
  events:
    - snsSqs:
        name: CalculateSessionMetrics
        topicArn: ${self:custom.eventsTopicArn}
        omitPhysicalId: true
        maxRetryCount: 3
        deadLetterQueueEnabled: true
        visibilityTimeout: 1800
        filterPolicy:
          name:
            - ....

I think https://github.com/agiledigital/serverless-sns-sqs-lambda/pull/813 fixes it, but figured it's best to open an issue too

lyuboraykov commented 8 months ago

@NoxHarmonium maybe you're the right person for this? Apologies if you're no longer involved, it would be great if you can help me find who is 🙏

lyuboraykov commented 8 months ago

@Tim-W-James @robinMcA @haolinj maybe you're the right people for this?

NoxHarmonium commented 8 months ago

Hi @lyuboraykov, I'm not involved much anymore, but since you were nice enough to provide a potential fix for the the issue I'll see if I can help you out 😄

My thoughts on https://github.com/agiledigital/serverless-sns-sqs-lambda/pull/813:

  1. I didn't know about this.serverless.providers.aws.naming.getNormalizedFunctionName, that's a good find, and would be much more robust.
  2. The snapshot tests are failing (https://github.com/agiledigital/serverless-sns-sqs-lambda/actions/runs/7709906552/job/21033152283) which points to this being a breaking change. It looks like queue names depend on the lambda name, so not only will lambdas be renamed but people's queues. We don't want people to update their packages and suddenly have all their infrastructure rename itself.

To deal with the breaking change we could either:

  1. Bump the major version of the library to indicate that there is a breaking change, and write an eye catching message in README.md to let people know they'll have to migrate.
  2. Allow people to opt-in to the new naming scheme using a config key (e.g. what we did here https://github.com/agiledigital/serverless-sns-sqs-lambda/pull/552/files)

I think we need to have to go with option two to avoid having a situation where people avoid upgrading to the next major version because the breaking changes are too hard to resolve in their stack and then we would have to support people on two different major versions. Also, people are generally bad at reading changelogs before upgrading and we might get a heap of people raising issues about it. It is annoying to have even more config options to deal with though. Maybe we could make it optional for now and make it default in a future version.

What do you think @lyuboraykov? Are you happy to put it behind a config option and write a quick test for it? You could use https://github.com/agiledigital/serverless-sns-sqs-lambda/pull/552/files as a template (it isn't as bad as it looks, a lot of it is snapshot updates!).