feat: add elasticache serverless support #101
Conversation
|
Hey @dollev36, thank you for another contribution! Currently, I'm very busy with the university, I will be able to review this PR in about two weeks. |
BohdanPetryshyn
left a comment
BohdanPetryshyn
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I played a bit with the implementation and have a couple of suggestions:
-
I noticed that your implementation works with both Redis and Memcached and this is very good! Could you please implement support for both cache engines in this PR? I think this will be beneficial for the public interface quality (CLI arguments and config) as well as for the code quality. I'd suggest the following properties for the public interface:
- CLI:
--elasticache-redis-serverless-cache,--elasticache-memcached-serverless-cache(in bothinitandconnect) - Config:
elasticacheRedisServerlessCache,elasticacheMemcachedServerlessCache. - Interactive output: (Currently, I see indentation problems in the output as well as slight inconsistencies with previously implemented cluster and node targets)
Elasticache targets: Redis serverless caches: my-redis-cache my-second-redis-cache Memcached serverless caches: my-memcached-cache - Primary endpoint my-memcached-cache - Reader endpoint my-second-memcached-cache - Primary endpoint my-second-memcached-cache - Primary endpoint Redis clusters: ...
- CLI:
-
Reader endpoint didn't work for Redis cache and I also can't see it in the AWS Web Console. I think there's no such concept for Redis serverless. The reader endpoint works for Memcached, though.
-
Please, update the corresponding docs pages: https://github.com/basti-app/basti/blob/main/docs/reference/cli.md, https://github.com/basti-app/basti/blob/main/docs/reference/configuration-file.md
| } | ||
|
|
||
| export async function getRedisServerlessCache( | ||
| Id: string, |
There was a problem hiding this comment.
Please, use lowercase for function arguments.
| securityGroupIds: string[]; | ||
| cachePreviousSecurityGroups: SecurityGroupMembership[]; | ||
| } | ||
| export interface ModifyServerlessInput { |
There was a problem hiding this comment.
Let's continue using Elasticache-specific names here. What about ModifyElasticacheServerlessInput?
| export function parseServerlessCacheResponse( | ||
| response: ServerlessCache | ||
| ): AwsElasticacheServerlessCache[] { | ||
| return [ |
There was a problem hiding this comment.
Please, use zod validator here.
|
Any updates on this? I just noticed that the serverless option is missing! As it would help me, I would be willing to finish the job, if you are still taking contributions. |
|
Hi @iainelder👋 I would really appreciate it if you could finish this PR! |
|
Can't make any promises about when. But if I find some time I'd love to finish this. |
|
Just wanted to say that I'm no longer working on a project that uses Elasticache Serverless. Until that changes, I probably won't do this. So if anyone else wants to finish the job, please go ahead. Basti continues to be a wonderful tool for connecting to RDS, and I'm still using it for that! Thanks. |
3 participants
Proposed Changes
add elasticache serverless support
Related Issues/PRs
Checklist
npm run test).