-
Notifications
You must be signed in to change notification settings - Fork 1
chore: redis v2 component #67
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
Conversation
MiroDojkic
left a comment
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.
Good work!
I see an opportunity for this to be two PRs - please try to reduce PR size in the future.
MiroDojkic
left a comment
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.
@droguljic I'll merge this to integrate it asap, but please review when you have time.
| export class UpstashRedis extends pulumi.ComponentResource { | ||
| instance: upstash.RedisDatabase; | ||
| password: Password; | ||
| username = 'default'; |
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 this needed? Where is it used?
|
|
||
| describe('EcsService component deployment', () => { | ||
| const region = process.env.AWS_REGION || 'us-east-2'; | ||
| const region = process.env.AWS_REGION; |
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.
What is the reason behind removing the default?
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.
My comment in the first code review. 👀
| const ecsServiceWithStorage = ctx.outputs.ecsServiceWithStorage.value; | ||
| const clusterName = ctx.outputs.cluster.value.name; | ||
| const region = process.env.AWS_REGION || 'us-east-2'; | ||
| const region = process.env.AWS_REGION; |
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.
What is the reason behind removing the default?
|
|
||
| describe('Web server component deployment', () => { | ||
| const region = process.env.AWS_REGION || 'us-east-2'; | ||
| const region = process.env.AWS_REGION; |
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.
What is the reason behind removing the default?
Added separate Redis components for AWS ElastiCache and Upstash. No shared base class since there's minimal common logic between them.
Changes:
UPSTASH_EMAILandUPSTASH_API_KEYenvironment variables