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

Ensure all relvant SQS/SNS CloudFormation attributes are mapped through #213

Closed
NoxHarmonium opened this issue Mar 4, 2021 · 7 comments · Fixed by #282
Closed

Ensure all relvant SQS/SNS CloudFormation attributes are mapped through #213

NoxHarmonium opened this issue Mar 4, 2021 · 7 comments · Fixed by #282
Labels

Comments

@NoxHarmonium
Copy link
Collaborator

Seems like people keep finding attributes that aren't mapped through:
E.g.

#205
#82
#28

We should go through and check if there are any more useful attributes we need to map through

@denise-sanders
Copy link

Is there a philosophy around which attributes you want to support? I was using your plugin and was thinking about implementing

  • enabled (I wrote a pr here feat: add enabled option #219)
  • queue retention on the main queue instead of just the dlq (although it's probably not that important for most people)
  • fifo queue support

@NoxHarmonium
Copy link
Collaborator Author

NoxHarmonium commented Mar 11, 2021

Is there a philosophy around which attributes you want to support?

I was thinking of just going through the CloudFormation documentation and mapping everything through or maybe just make it more dynamic so we can just pass arbitrary properties through to the CloudFormation template, but that sounds error prone.
Until we find the time to do that, every bit counts though!

enabled (I wrote a pr here #219)

Thanks we'll have a look 🚀

fifo queue support

That sounds important, that might be a good next one to map through.

@NoxHarmonium
Copy link
Collaborator Author

There are heaps of useful parameters on the event mapping too that we need to map through (or allow some override)

BisectBatchOnFunctionError in particular is super useful

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-eventsourcemapping.html

@NoxHarmonium
Copy link
Collaborator Author

Oh it turns out that "BisectBatchOnFunctionError" is only a feature if you're subscribing to Kinesis streams.

NoxHarmonium added a commit that referenced this issue May 2, 2021
- We keep having to add new options to the config when someone needs to use an option in the CloudFormation template we haven't mapped yet
- This should future proof the plugin to allow users to just pass whatever they need through to override the generated template
- Eventually we could also deprecate and remove some of the other options that could just be overrides
- Fixes #213
@NoxHarmonium
Copy link
Collaborator Author

@robinMcA @dbalmain what are your thoughts on https://github.com/agiledigital/serverless-sns-sqs-lambda/tree/pass-through-cf-params ?
If you like that approach I can clean it up and raise a PR.

@dbalmain
Copy link
Collaborator

dbalmain commented May 2, 2021

Love it! This is a great idea.

NoxHarmonium added a commit that referenced this issue May 12, 2021
- We keep having to add new options to the config when someone needs to use an option in the CloudFormation template we haven't mapped yet
- This should future proof the plugin to allow users to just pass whatever they need through to override the generated template
- Eventually we could also deprecate and remove some of the other options that could just be overrides
- Fixes #213
NoxHarmonium pushed a commit that referenced this issue May 14, 2021
# [0.6.0](v0.5.0...v0.6.0) (2021-05-14)

### Features

* allow CloudFormation overrides ([6d80b18](6d80b18)), closes [#213](#213)
@NoxHarmonium
Copy link
Collaborator Author

🎉 This issue has been resolved in version 0.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants