SNS Tags update #222
SNS Tags update #222
Conversation
| snsClient: SNSClient | ||
| stsClient: STSClient |
There was a problem hiding this comment.
new dependency as discussed
| describe('buildTopicArn', () => { | ||
| it('build ARN for topic', async () => { | ||
| const topicName = 'my-test-topic' | ||
| await expect(buildTopicArn(stsClient, topicName)).resolves.toMatchInlineSnapshot( |
There was a problem hiding this comment.
might be good to also create a topic with the same name within the test, and compare actual arn to manually built one. then we will break if AWS will change the structure at some point
There was a problem hiding this comment.
Good idea! will add it 🙌 thanks igor 🙇
packages/sns/lib/utils/stsUtils.ts
Outdated
| * Doc -> https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html | ||
| */ | ||
| export const buildTopicArn = async (client: STSClient, topicName: string) => { | ||
| const identityResponse = await client.send(new GetCallerIdentityCommand({})) |
There was a problem hiding this comment.
I would move this call outside of the function and make building itself synchronous, but also expose an asynchronous method on top of it that does resolution for you.
from performance perspective it would be ideal to only resolve the identity once per app startup and not once per every queue. for now we can rely on async one, but having sync one exposed as well would allow to optimize in the future by moving identity resolution to app itself
There was a problem hiding this comment.
I am not sure if moving identity resolution to the app is a good idea 🤔, maybe we can do it once and cache the result so the next calls won't need it, we can even do it within this file, what do you think?
There was a problem hiding this comment.
sounds good! but let's expose a method to reset it as well, for tests e. g.
| return `arn:aws:sns:${region}:${identityResponse.Account}:${topicName}` | ||
| } | ||
|
|
||
| export const clearCachedCallerIdentity = () => { |
There was a problem hiding this comment.
let's expose it from package exports as well, because users may need it too
There was a problem hiding this comment.
Good point! I forgot to expose it on index 😓
No description provided.