-
Notifications
You must be signed in to change notification settings - Fork 24
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(bus-sqs): use aws-sdk v3, allow passing ARNs #199
base: aws-sdk-3
Are you sure you want to change the base?
Conversation
hi @shellscape - thanks for the PR! just letting you know I'm reviewing this now. The proposal's really smart and I'll merge this through if there are no issues |
@@ -157,7 +202,7 @@ export class SqsTransport implements Transport<SQS.Message> { | |||
) | |||
await Promise.allSettled( | |||
result.Messages | |||
.map(async message => this.makeMessageVisible(message)) | |||
.map(async (message: any) => this.makeMessageVisible(message)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be cast to any
? It looks like the implict type is correct
@@ -224,7 +269,7 @@ export class SqsTransport implements Transport<SQS.Message> { | |||
} | |||
) | |||
|
|||
const serviceQueueAttributes: QueueAttributeMap = { | |||
const serviceQueueAttributes: Record<string, string> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to align this with the target type? something like:
const serviceQueueAttributes: Record<string, string> = { | |
const serviceQueueAttributes: CreateQueueCommandInput['Attributes'] = { |
@@ -265,17 +310,17 @@ export class SqsTransport implements Transport<SQS.Message> { | |||
*/ | |||
private async assertSqsQueue ( | |||
queueName: string, | |||
queueAttributes?: QueueAttributeMap | |||
queueAttributes?: Record<string, string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto from above 🙂
queueAttributes?: Record<string, string> | |
queueAttributes?: CreateQueueCommandInput['Attributes'] |
hey @shellscape the changes look great! I've left a couple of nit-picky comments, but the structure is all fine. One small blocker is the |
hey @adenhertog thanks for taking a look. yes, integration tests didn't work out of the box on a fresh clone, and docs didn't seem to address the appropriate setup for getting them to run, so in the spirit of providing code for aws-sdk v3 to the community, we opened this PR as is. since I opened this up we've come to find out that much of the mechanisms of the default sqs bus require blanket permissions in the running context to both SQS and SNS, and assume that those permissions are available (e.g. as a result, we've mostly ripped out the SNS functionality and the automatic creation of queues. that ends up being quite the divergence from the transports in this repo. you can view our changes along those lines here if you're curious https://github.com/Moment-Wealth/bus/tree/%40superadvisor. given the assumptions about AWS that were made in the exisiting sqs bus and the large divergence of the code in our branch, we're not going to continue work on |
hey @shellscape thanks for the debrief and for raising this MR to demonstrate the aws-sdk-v3 changes. I'll still get that part merged into master as it's certainly useful. I have had to reapply the changes in a separate branch as the repo has changed a bit in the meantime. It's been a month since you commented, but if you still remember the context I'd be keen to learn a bit more about two of your points:
To confirm, this is saying that the IAM policies needed to bootstrap the app is different to those needed to run the app (ie: creation/subscription of queues vs publishing & deleting messages). I do agree here and the current approach is more of a short-cut. Would something like a db-migration style approach fit your use-case here? eg: the build server can have the bootstrap IAM policy and then run a cli-command to create/subscribe queues; meaning the runtime policy can be more restrictive.
This approach was mainly to decouple the sender from having knowledge of the receiver. For a more general library like this I can't see much benefit to dropping the SNS support, but I'm guessing you have a more optimised use case. Keen to learn what this is if you can share :) |
This PR proposes the following:
bus-sqs
to use AWS SDK v3queueArn
instead ofawsAccountId
andawsRegion
deadLetterQueueArn
This update has the advantage of producing smaller bundles, as v3 is tree-shakable, as well as automatic discovery of AWS environment and credentials data (a built-in with v3). This PR also makes this package much smaller as the latest v2 aws-sdk weighs in at a massive 77mb https://packagephobia.com/result?p=aws-sdk.
I had opened #198 to discuss, but as we're in immediate need on our end, I figured it wouldn't hurt to open a PR as well.