-
Notifications
You must be signed in to change notification settings - Fork 21
fix(aws-elasticache)!: correct major engine version cannot be set on serverless cache for memcached #77
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
base: main
Are you sure you want to change the base?
fix(aws-elasticache)!: correct major engine version cannot be set on serverless cache for memcached #77
Conversation
| interface EngineVersionBaseProps { | ||
| /** | ||
| * The major version of the engine. | ||
| */ | ||
| readonly majorVersion: 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.
Created the props instead of directly using majorVersion as arguments for the EngineVersion in case properties such as fullVersion are added in the future.
| /** | ||
| * Properties for the Valkey engine version. | ||
| */ | ||
| export interface ValkeyEngineVersionProps extends EngineVersionBaseProps {} |
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.
Considering the possibility that properties such as fullVersion may be added only for partial engines in the future, I have prepared props for each engine: ValkeyEngineVersionProps, RedisEngineVersionProps and MemcachedEngineVersionProps.
| /** | ||
| * Properties of the Valkey engine for serverless cache. | ||
| */ | ||
| export interface ValkeyEngineProps { | ||
| /** | ||
| * The engine version of the Valkey engine. | ||
| */ | ||
| readonly engineVersion: ValkeyEngineVersion; | ||
| } |
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.
The EngineVersion class (property) is wrapped in the EngineProps for factory methods of the ServerlessCacheEngine to allow for the possibility of additional elements other than the version in the future.
…o elasticache-serverless-engine
Issue # (if applicable)
Closes #76 .
Reason for this change
Correct major engine version cannot be set on serverless cache for memcached.
Currently the same
MajorVersionproperty is used for all the engine.https://github.com/open-constructs/aws-cdk-library/blob/v0.1.0/src/aws-elasticache/serverless-cache.ts#L69-L79
But the major version
1.6can only be specified for memcached.And
7and8for valkey and7for redis only can be specified.Supported versions: https://docs.aws.amazon.com/AmazonElastiCache/latest/dg/engine-versions.html
FYI: https://docs.aws.amazon.com/AmazonElastiCache/latest/dg/VersionManagement.html
Description of changes
Two approaches can be applied for the issue:
1. Adding a new version (
1.6/VER_1_6) for memcached in the existing enum propertyThis approach is adding the new version
VER_1_6to the existing enum.But additional validation process for correct combinations is required and the usability is not good because users can choose any of the options even though different engines have different possible versions.
export enum MajorVersion { /** - * Version 7 + * Version 7 for Valkey and Redis only */ VER_7 = '7', /** - * Version 8 + * Version 8 for Valkey only */ VER_8 = '8', + + /** + * Version 1.6 for Memcached only + */ + VER_1_6 = '1.6', }2. Adding a new property for each engine, including version
I decided to use this approach in this PR.
This approach removes the existing
engineandmajorEngineVersionproperties and adds a newserverlessCacheEngineproperty using Enum-like classes.This would be a breaking change since it would remove existing properties, but we are still on version 0 so it is doable. I think it is too early to adopt Approach 1 for backward compatibility concerns. Approach 1 is not a good approach and is inconvenient for users.
Also, those old properties are required properties now, so we CANNOT use the feature flag alternative.
Description of how you validated changes
Both unit tests and integ tests.
Checklist
aws-part in the scope of the PR title if the PR relates to a specific AWS service module.Breaking Change
BREAKING CHANGE: The existing properties
engineandmajorEngineVersionforServerlessCachewill be removed, users will have to use the new propertyserverlessCacheEngine.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license