SNS Tag update #221
SNS Tag update #221
Conversation
| * It follows the following pattern: arn:aws:sns:<region>:<account-id>:<topic-name> | ||
| * Doc -> https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html | ||
| */ | ||
| const buildTopicArn = async (client: SNSClient, topicName: string) => { |
There was a problem hiding this comment.
I would expose this from utils, this can be useful in a variety of different use-cases.
Would be nice to also write some tests for it, to ensure that the built arn matches the real arn created by SNS based on same parameters
There was a problem hiding this comment.
Mmm, Okay! will expose it 🙏
| const region = | ||
| typeof client.config.region === 'string' ? client.config.region : await client.config.region() | ||
|
|
||
| const stsClient = new STSClient({ |
There was a problem hiding this comment.
recreating client on every request sounds like a lot of overhead, I'd suggest to pass it as a dependency instead
There was a problem hiding this comment.
Do you mean as a dependency for users? the idea was to hide its usage by using SNS config parameters, although I am not really sure if it is a good idea 🤔.
What do you mean by recreating it in every request? I think it is done just once on init right?
There was a problem hiding this comment.
Oh I got it, you mean to the method, would be fine to create it on publisher/consumer creation and pass it as parameter?
There was a problem hiding this comment.
wouldn't it be done once per buildTopicArn, which realistically means once for every topic?
There was a problem hiding this comment.
yeah, same as we pass sqsClient and snsClient
There was a problem hiding this comment.
But as a dependency passed by the user, or do you think it is a good idea to build it internally base on SNS config? Advantage of not requesting it is that the API won’t change
There was a problem hiding this comment.
(Btw sorry, I am already a bit slow 😓)
There was a problem hiding this comment.
I would pass by user, would be less surprising than globally cached dependency. but we can also make an optional parameter and build automatically if not passed
There was a problem hiding this comment.
Makes sense to me! Will think about it and address the change in the new PR
Thanks Igor 🙌🏽
|
Will close this in favour of #222 to have clean PR, this is a bit dirty with all tests |
No description provided.